mozilla :: #pdfjs

17 Apr 2017
12:32github_pdfjs[pdf.js] yurydelendik pushed 2 new commits to master:
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).
12:34github_pdfjs[pdf.js] yurydelendik pushed 2 new commits to master:
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).
12:53github_pdfjs[pdf.js] pdfjsbot force-pushed gh-pages from 752f396 to efe2e7f:
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:
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
15:59yuryokay, you on a right track
16:00mukulalso pushed the streams-getTextContent branch
16:00mukuland it passes all the unittests
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:21mukulyou mean something like
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: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: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: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:49mukulI changed getTextContent to
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
18:59yurymukul: looks fine, but you can improve this by
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: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
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
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: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
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 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: 6 days and 9 hours ago