mozilla :: #pdfjs

17 Apr 2017
12:32github_pdfjs[pdf.js] yurydelendik pushed 2 new commits to master: https://github.com/mozilla/pdf.js/compare/bd0e4dc4e302...7812b3cfb216
12:32github_pdfjspdf.js/master 79e0745 Jonas Jenwald: Import `getGlobalEventBus` correctly from `web/dom_events.js` in various `/web` files, to un-break e.g. the viewer components (PR 8203 follow-up)...
12:32github_pdfjspdf.js/master 7812b3c Yury Delendik: Merge pull request #8299 from Snuffleupagus/domEvents-getGlobalEventBus-import...
12:32soakbotPull 8299: Import `getGlobalEventBus` correctly from `web/dom_events.js` in various `/web` files, to un-break e.g. the viewer components (PR 8203 follow-up). https://github.com/mozilla/pdf.js/pull/8299
12:34github_pdfjs[pdf.js] yurydelendik pushed 2 new commits to master: https://github.com/mozilla/pdf.js/compare/7812b3cfb216...5ad3611cc436
12:34github_pdfjspdf.js/master 594e8c0 Jonas Jenwald: Refactor the `DownloadManager` initialization in `GENERIC`/`CHROME` builds again (PR 8203 follow-up)...
12:34github_pdfjspdf.js/master 5ad3611 Yury Delendik: Merge pull request #8300 from Snuffleupagus/GENERIC-DownloadManager-creation...
12:34soakbotPull 8300: Refactor the `DownloadManager` initialization in `GENERIC`/`CHROME` builds again (PR 8203 follow-up). https://github.com/mozilla/pdf.js/pull/8300
12:53github_pdfjs[pdf.js] pdfjsbot force-pushed gh-pages from 752f396 to efe2e7f: https://github.com/mozilla/pdf.js/commits/gh-pages
12:53github_pdfjspdf.js/gh-pages efe2e7f pdfjsbot: gh-pages site created via gulpfile.js script...
14:03github_pdfjs[pdf.js] pdfjsbot force-pushed gh-pages from efe2e7f to ec14581: https://github.com/mozilla/pdf.js/commits/gh-pages
14:03github_pdfjspdf.js/gh-pages ec14581 pdfjsbot: gh-pages site created via gulpfile.js script...
15:40mukulyury: Hi
15:56yurymukul: hi
15:57mukulHere is the code for getTextContent with sendWithStream you are asking me earlier https://gist.github.com/mukulmishra18/6898c7c7c4676c0e6ad6ab139a84665e
15:59yuryokay, you on a right track
16:00mukulalso pushed the streams-getTextContent branch https://github.com/mukulmishra18/pdf.js/tree/streams-getTextContent
16:00mukuland it passes all the unittests
16:02yurymukul: https://pastebin.mozilla.org/9019219
16:03yuryeven https://pastebin.mozilla.org/9019220
16:05yurymukul: I don't see you are passing highWaterMark param and size function
16:06yurybut it's okay for sending one-by-one item
16:07mukulI console.log something inside the size function and it logging
16:07mukulI have some doubt how it is working exactly
16:07mukulI think it is called automatically when we enqueue the data
16:07mukulor when we call pull
16:08yurydesiredSize is calculated base on how much you enqueued `highWaterMark - size(chunk)`
16:09yuryyou don't have to send back exact desiredSize
16:09mukulOh, okay. That mean it is calling size function to cal desiredSize.
16:10yurysmall steps, can you change your example to add highWaterMark and address my pastebin comments
16:12mukulOkay, then i have to pass highWaterMark to worker
16:12mukuli.e. getTextContent handler
16:13yurynope, it shall be passed to ReadableStream
16:15mukulHmm..., then i think i miss interpreted `add highWaterMark` in your above message
16:17* yury is checking the spec
16:18yury"The constructor also accepts a second argument containing the queuing strategy object with two properties: a non-negative number highWaterMark, and a function size(chunk).
16:18yury"
16:21mukulyou mean something like https://github.com/mukulmishra18/pdf.js/commit/e2e95437d0a826bc2b4f29645a6df07ef7de1900#diff-8d2b578d58d4d8c8a2975020fbbc84d9R1496
16:21mukul?
16:21yurywhat's arr?
16:21yurysendWithStream shall accept that as a parameter
16:22mukularray we are passing to enqueue
16:22mukulwhen pull is called
16:23mukulokay, we can modify it so that sendWithStream will accept it as param
16:23yuryit will chunk-object, which will have styles and items properties -- it will not be an array
16:24mukulyury: actually i am breaking the textContent object and first passing the styles and the items
16:25mukulhere https://github.com/mukulmishra18/pdf.js/commit/f9cc0d9ee106d4b7a02fb57e5aed08d6e5232b20#diff-780c33139a29f60c0cc45b80fb05f900R925
16:26yurycan you break and return {styles:..., items: []} where items is few items from the result?
16:26yuryfirst chunk styles will have all styles
16:26yurythe following will be empty
16:27mukulHmm..., yes we do so.
16:27mukulBut is there any logic behind it?
16:28yuryin the future you can add styles with *only* new entries for chunk items
16:29yuryso in most of the cases styles object will be empty if few styles are used
16:31mukulokay, how size function will change according to that?
16:32yuryif `return items.splice(0, desiredSize);` will be changed to `return {styles: {}, items: items.splice(0, desiredSize};`?
16:33yurychunk will appear is {styles: {}, items: []} on the other side (and inside size function)
16:33yuryappear as
16:35yurymukul: so how size function will look if we will wrap chunk as {styles: {}, items: []} ?
16:36mukulif we don't care about styles and pass whole the styles in first pull
16:36yurywe don't care about styles size
16:36mukulThen we just have to do something like textContent.items.length
16:36mukulsize(textContent)
16:37yuryright
16:37mukulHmm..., great.
16:37yuryI'm thinking that size is not called since onPull does not return promise
16:38mukulThen should i change the pull function also?
16:38yurylots of work ahead with sendWithStream, so I would like to see correct getTextContent snippet first
16:39yuryonce we agree on that, let's prototype other side (action handler)
16:40yuryafter that sendWithStream will be easy
16:40mukulI think in getTextContent i have to just address you comments and change sendWithStream to accept queueing strategy
16:40yurycool
16:41* yury will wait for final version and prototype for action handler
16:42mukulcoming to you after sometimes with a solution :)
16:42mukulhold on :-D
18:48mukulyury: ping
18:48yurypong
18:49mukulI changed getTextContent to https://pastebin.mozilla.org/9019239
18:50mukulbut it is calling pump() even promise is resolved
18:50yuryyou don't have to call pump() after resolve
18:51yurymukul: otherwise code looks great
18:51yurydid you start modification of action handler?
18:51mukulOkay, just a sec
18:52mukultake a look https://pastebin.mozilla.org/9019242
18:59yurymukul: looks fine, but you can improve this by https://pastebin.mozilla.org/9019244
19:00yurymukul: and probably I would move getChunk logic inside onPull and expose items/styles instead
19:02mukulHmm...., seems to be more logical
19:02yurymukul: now, start fixing sendWithStream (add queue options, use capability to block `pull` and `cancel`callback)
19:04mukulqueue options?
19:04mukulmeans queueing strategy?
19:04yuryright
19:12mukulyury: i think we need to call resolve inside the handler because it notify that textContent is ready to pull
19:13yurymukul: do you return a promise returned by .then()
19:13yuryevery then() method returns a promise
19:13yurymukul: see line from https://pastebin.mozilla.org/9019244
19:14mukuloh, okay you returned then
19:14mukuli got it
19:14yurythis way to can chain promises,e.g. promise.then(...).then(...)
19:14mukulyes, sure
19:14yurythose will be resolved to whatever is returned
19:28mukulyury: after addressing your comments https://pastebin.mozilla.org/9019248
19:29yurysomething wrong with reject(reason);
19:30mukulOh, yes.
19:30yuryand onCancel can be empty
19:30mukulWe don't want reject
19:31mukulwe don't want anything in onCancel for now?
19:31yurynope
19:31yuryfor reject, see original code `throw reason;`
19:31yuryall you needed to get rid of 'return textContent;'
19:32yuryfor now we will fail on start
19:33yurymukul: you can publish this code on getTextContent branch
19:33mukulOkay, i will push this modifications
19:35mukulyury: `we will fail on start` why?
19:36yuryif extracttextcontent fails we need to fail the stream
19:36yurylater it will be during read operations though
19:37yuryfor now, we will just inform the start promise
19:38mukulOkay, yes. You are right.
19:39mukuldo you think instead of `throw reason`, we can use something like `throw new Error(reason)`
19:39mukulin handler
19:40yurythe reason shall be an Error
19:40mukulokay, so it is an instance of error.
19:40mukulthen it is fine to have throw error.
20:00mukulyury: I pushed the changes in streams-getTextContent branch https://github.com/mukulmishra18/pdf.js/commit/876dd5bec94e50f7e9366d9e2da103f0372e71bb
20:00mukulstarting to work on sendWithStream
20:00yuryokay, short enough
20:01yurymukul: can you also move extra stuff to the other branch
20:02mukulextra stuff?
20:02yuryso you can type move back to the sendWithStream branch and add https://github.com/mukulmishra18/pdf.js/commit/876dd5bec94e50f7e9366d9e2da103f0372e71bb#diff-8d2b578d58d4d8c8a2975020fbbc84d9 stuff there
20:03yurythen rebase on top of new changes streams-getTextContent
20:04yuryideally sendWithStream code/branch shall have tests that check functionality of start/pull/cancel logic
20:04yuryand queueing strategy
20:05mukulOkay, sounds right to have above code in sendWithStream branch
20:06yuryit is really hard to add tests later
20:08mukulHmm...., i will try to add tests as well
20:10mukulwe have test for sendWithStream right now, but it is not testing it's all functionality
20:12mukulwe can add tests to test it's functionality whenever we add new
18 Apr 2017
No messages
   
Last message: 157 days and 14 hours ago