[api-minor] Add a |cancel| method to PDFDocumentLoadingTask #5629
Conversation
|
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/93203fc5f3b68f7/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/708c990f1c84eef/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/708c990f1c84eef/output.txt Total script time: 17.43 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/93203fc5f3b68f7/output.txt Total script time: 23.09 mins
|
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/222a03cb5ec4912/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/222a03cb5ec4912/output.txt Total script time: 0.77 mins Published
|
| 'An error occurred while loading the PDF.'); | ||
|
|
||
| if (exception instanceof PDFJS.CancelGetDocumentException) { | ||
| loadingErrorMessage = mozL10n.get('cancel_get_document_error', null, |
There was a problem hiding this comment.
I don't think we need to show any errors to user -- first, it was his decision in first place and, second, it may confuse the user if he chooses to load a different document and he received an error instead.
There was a problem hiding this comment.
I agree; which is why the code below just prints a message in the console, and then returns immediately thus avoiding triggering error.
Hence the only thing the user could potentially see, is a message logged in the console.
Are you saying that we shouldn't even print anything in the console, and just return straight away instead?
There was a problem hiding this comment.
I would say not even a console message, but let's print info message just for troubleshooting.
|
It's more complex than I thought. We need to cancel it not only on the api.js side, but on the worker.js side as well. Which means any in-progress xhr must be terminated, any rendering operations terminated. Also, we probably need to add a callback similar to onProgress, on signify end of document loading (onComplete?) -- it might not fire on disableAutoFetch, but it will signify end of |
Maybe just terminate xhr and will not allow any I/O operations after that. PDFDocumentProxy.destroy shall worry about worker cleanup and aborting operations. |
Isn't the patch already doing this, at least partially? See https://github.com/mozilla/pdf.js/pull/5629/files#diff-0ecad279ed2252e3eb47a4d96ec1463cR270 (which might need a slightly better comment). Since api.js#L951 calls worker.js#L459, which in turns calls pdf_manager.js#L218 and then finally network.js#L262. As far as I can tell, this should be sufficient to close any open connections. |
|
Yes. But we don't have exposed api for it at getDocument level. Also this api call shall not destroy the worker if getDocument is successfully resolved - we have destroy in PDFDocumentProxy. So this PR will be about refactoring mess with states we have atm |
Snuffleupagus
changed the title
(Replaces PR #5234.)
Fixes #3485.
Fixes #4774.
Fixes #5228.
Fixes #5572.