[api-minor] Change PDFDocumentProxy.cleanup/PDFPageProxy.cleanup to return data
#11573
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Snuffleupagus
force-pushed
the
api-cleanup-returns
branch
2 times, most recently
from
e982d8d to
349d0f9
Compare
|
/botio unittest |
From: Bot.io (Linux m4)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/e80be8d1160bd54/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/2ac32d741ddc57a/output.txt |
…to return data This patch makes the following changes, to improve these API methods: - Let `PDFPageProxy.cleanup` return a boolean indicating if clean-up actually happened, since ongoing rendering will block clean-up. Besides being used in other parts of this patch, it seems that an API user may also be interested in the return value given that clean-up isn't *guaranteed* to happen. - Let `PDFDocumentProxy.cleanup` return the promise indicating when clean-up is finished. - Improve the JSDoc comment for `PDFDocumentProxy.cleanup` to mention that clean-up is triggered on *both* threads (without going into unnecessary specifics regarding what *exactly* said data actually is). Add a note in the JSDoc comment about not calling this method when rendering is ongoing. - Change `WorkerTransport.startCleanup` to throw an `Error` if it's called when rendering is ongoing, to prevent rendering from breaking. Please note that this won't stop *worker-thread* clean-up from happening (since there's no general "something is rendering"-flag), however I'm not sure if that's really a problem; but please don't quote me on that :-) All of the caches that's being cleared in `Catalog.cleanup`, on the worker-thread, *should* be re-filled automatically even if cleared *during* parsing/rendering, and the only thing that probably happens is that e.g. font data would have to be re-parsed. On the main-thread, on the other hand, clearing the caches is more-or-less guaranteed to cause rendering errors, since the rendering code in `src/display/canvas.js` isn't able to re-request any image/font data that's suddenly being pulled out from under it. - Last, but not least, add a couple of basic unit-tests for the clean-up functionality.
Snuffleupagus
force-pushed
the
api-cleanup-returns
branch
from
349d0f9 to
7117ee0
Compare
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/e80be8d1160bd54/output.txt Total script time: 2.73 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/2ac32d741ddc57a/output.txt Total script time: 4.81 mins
|
Snuffleupagus
commented
Feb 8, 2020
timvandermeij
approved these changes
Feb 8, 2020
|
This looks like a useful API addition. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch makes the following changes, to improve these API methods:
Let
PDFPageProxy.cleanupreturn a boolean indicating if clean-up actually happened, since ongoing rendering will block clean-up.Besides being used in other parts of this patch, it seems that an API user may also be interested in the return value given that clean-up isn't guaranteed to happen.
Let
PDFDocumentProxy.cleanupreturn the promise indicating when clean-up is finished.Improve the JSDoc comment for
PDFDocumentProxy.cleanupto mention that clean-up is triggered on both threads (without going into unnecessary specifics regarding what exactly said data actually is).Add a note in the JSDoc comment about not calling this method when rendering is ongoing.
Change
WorkerTransport.startCleanupto throw anErrorif it's called when rendering is ongoing, to prevent rendering from breaking.Please note that this won't stop worker-thread clean-up from happening (since there's no general "something is rendering"-flag), however I'm not sure if that's really a problem; but please don't quote me on that :-)
All of the caches that's being cleared in
Catalog.cleanup, on the worker-thread, should be re-filled automatically even if cleared during parsing/rendering, and the only thing that probably happens is that e.g. font data would have to be re-parsed.On the main-thread, on the other hand, clearing the caches is more-or-less guaranteed to cause rendering errors, since the rendering code in
src/display/canvas.jsisn't able to re-request any image/font data that's suddenly being pulled out from under it.Last, but not least, add a couple of basic unit-tests for the clean-up functionality.