Miscellaneous API clean-up #9781
Conversation
…r._setupFakeWorker` With version 2.0, native support for typed arrays is now a requirement for using the PDF.js library; see PR 9094 where the old polyfills were removed. Hence the `isTypedArraysPresent` check, when setting up fake workers, no longer serves any purpose here and can thus be removed.
The signature of the `PDFWorker.fromPort` method, in addition to the `PDFWorker` constructor, was changed in PR 9480. Hence it's probably a good idea to add a bit more validation to `PDFWorker.fromPort`, to ensure that it won't fail silently for an API consumer that updates to version 2.0 of the PDF.js library.
…in src/display/api.js Since all the built-in PDF.js image decoders now return their data as `Uint8ClampedArray`, for consistency `JpegDecode` on the main-thread should be doing the same thing; follow-up to PR 8778.
…he worker if typed arrays aren't properly supported
With native typed array support now being mandatory in PDF.js, since version 2.0, this probably isn't a huge problem even though the current code seems wrong (it was changed in PR 6571).
Note how in the `!(data instanceof Uint8Array)` case we're currently attempting to send `handler.send('test', 'main', false);` to the main-thread, which doesn't really make any sense since the signature of the method reads `send(actionName, data, transfers) {`.
Hence the data that's *actually* being sent here is `'main'`, with `false` as the transferList, which just seems weird. On the main-thread, this means that we're in this case checking `data && data.supportTypedArray`, where `data` contains the string `'main'` rather than being falsy. Since a string doesn't have a `supportTypedArray` property, that check still fails as expected but it doesn't seem great nonetheless.
…s unused in the API After PR 8617 the `PDFManagerReady` message handler function, in `src/display/api.js`, is now a no-op. Hence it seems completely unnecessary to keep sending this message from `src/core/worker.js`.
…ndicate that the property should be considered "private" Since `PDFDocumentProxy` already provide getters for all the data returned by `GetDoc` (in the Worker), there isn't any compelling reason for accessing the `pdfInfo` directly on `PDFDocumentProxy`.
…e that the property should be considered "private" Since `PDFPageProxy` already provide getters for all the data returned by `GetPage` (in the Worker), there isn't any compelling reason for accessing the `pageInfo` directly on `PDFPageProxy`. The patch also changes the `GetPage` handler, in `src/core/worker.js`, to use modern JavaScript features.
…in the `PDFWorker` constructor (PR 9480 follow-up)
…er in `getDocument` This special handling was added in PR 8567, but was made redundant in PR 8721 which stopped sending everything but the kitchen sink to the Worker side.
…in the `src/core/worker.js` file Since the old comment mentions a now unsupported browser, let's update it such that someone won't accidentally conclude that the code in question can be removed.
…js` file The various classes have `this._errored` and `this._reason` properties, where the first one is a boolean indicating if an error was encountered and the second one contains the actual `Error` (or `null` initially). In practice this means that errors are basically tracked *twice*, rather than just once. This kind of double-bookkeeping is generally a bad idea, since it's quite easy for the properties to (accidentally) get into an inconsistent state whenever the relevant code is modified. Rather than using a separate boolean, we can just as well check the "error" property directly (since `null` is falsy). --- Somewhat unrelated to this patch, but `src/display/node_stream.js` is currently *not* handling errors in a consistent or even correct way; compared with `src/display/network.js` and `src/display/fetch_stream.js`. Obviously using the `createResponseStatusError` utility function, from `src/display/network_utils.js`, might not make much sense in a Node.js environment. However at the *very* least it seems that `MissingPDFException`, or `UnknownErrorException` when one cannot tell that the PDF file is "missing", should be manually thrown. As is, the API (i.e. `getDocument`) is not returning the *expected* errors when loading fails in Node.js environments (as evident from the `pending` API unit-test).
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/50dc5697786979e/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/65c217dfe774158/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/50dc5697786979e/output.txt Total script time: 18.98 mins
|
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/65c217dfe774158/output.txt Total script time: 23.65 mins
Image differences available at: http://54.215.176.217:8877/65c217dfe774158/reftest-analyzer.html#web=eq.log |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/3654395f65d15eb/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/3654395f65d15eb/output.txt Total script time: 3.01 mins Published |
…src/display/dom_utils.js` Not only is the `Util.loadScript` helper function unused on the Worker side, even trying to use it there would throw an Error (since `document` isn't defined/available in Workers). Hence this helper function is moved, and its code modernized slightly by having it return a Promise rather than needing a callback function. Finally, to reduced code duplication, the "new" loadScript function is exported and used in the viewer.
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/94f02c3a2b43908/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/94f02c3a2b43908/output.txt Total script time: 2.92 mins Published |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/edc325243f4efa7/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/880a4240182108f/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/880a4240182108f/output.txt Total script time: 19.03 mins
|
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/edc325243f4efa7/output.txt Total script time: 23.63 mins
Image differences available at: http://54.215.176.217:8877/edc325243f4efa7/reftest-analyzer.html#web=eq.log |
|
Thank you for working on this! I have filed a follow-up issue for the error handling. |
|
/botio-windows makeref |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @timvandermeij received. Current queue size: 1 Live output at: http://54.215.176.217:8877/a7917d7f0ef8843/output.txt |
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/a7917d7f0ef8843/output.txt Total script time: 21.26 mins
|
Miscellaneous API clean-up
This patch series contains various (small) clean-up of API related code. I just recently found this locally and initially thought I'd submitted it as part of the PDF.js version
2.0project, however I'd apparently missed to do so; better late than never I suppose :-)/cc @timvandermeij
It'd probably be a good idea to file a follow-up to fix the pre-existing issues with the Error handling in the
node_stream.jsfile, as outlined in the last commit message: