A number of these unit-tests didn't actually cover the intended code-paths, since many of them *accidentally* matched the "file size is smaller than two range requests"-check.
The patch also updates `validateRangeRequestCapabilities` to use return-value names that are consistent with the class fields used in the various stream implementations.
With these changes `0`, `NaN`, `null`, and `undefined` in the `total`-property all result in `percent === NaN` being reported by the callback, since previously e.g. `0` would result in `percent === 100` being reported unconditionally which doesn't make a lot of sense.
Also, remove the "indeterminate" loadingBar (in the viewer) if the `PDFDocumentLoadingTask` fails since there won't be any more data arriving and displaying the animation thus seems wrong.
The user has to select some pages and then click on the "Save As" menu item in the Manage menu.
If they modify the structure of the pdf (deleted, moved, copied pages), they have to use the usual
save button.
This solves [bug 1989406](https://bugzilla.mozilla.org/show_bug.cgi?id=1989406).
(“The user should be able to dismiss the in-content message displayed by clicking somewhere else in the PDF”)
There’s a good gif there that shows the problematic behavior.
In the thread, there are also mentions of 2 similar but slightly separate problems:
* clicking on another highlight should also dismiss
* the mention that hitting the escape key does not dismiss
I found the last point, the escape key, to work already (first test case here).
But this PR solves the main bug (second test case) and the adjacent one
(third test case).
It works by using the existing `unselectAll` handling.
This will ignore filenames that become effectively empty, i.e. ones that are only ".pdf" and nothing more.
*Please note:* While this passes all existing unit-tests, I don't know if this is necessarily the "correct" solution here.
Currently the transfers aren't actually being used with the "GetOperatorList" message, since the placement of the parameter is wrong; note the method signature: 909a700afa/src/shared/message_handler.js (L219-L229)
This goes back to PR 16588, which added the transfers parameter, and unfortunately we all missed that :-(
Simply fixing the parameter isn't enough however, since that broke printing of Stamp-editors (and possibly others). The solution here is to *not* transfer data during printing, given that a single `PrintAnnotationStorage` instance is being used for all pages.
Change all these cases to use `Map.prototype.getOrInsertComputed()` instead, in combination with a helper function for creating the `Array`s (similar to the previous patch).
With the exception of the first invocation the callback function is unused, which means that a lot of pointless functions may be created.
To avoid this we introduce helper functions for simple cases, such as creating `Map`s and `Objects`s.
Currently we essentially "duplicate" the same code for parsing the `values` and `keys` of the `searchParams`, which seems a little unnecessary.
To be able to parse the `searchParams` from the end, we currently create an Array (from the Iterator) and then reverse it before finally looping through it. Here the latter two steps can be replaced with the `Array.prototype.findLast()` method instead.
*Please note:* I completely understand if this patch is rejected, on account of being less readable than the current code.
This leads to slightly shorter code[1] when initializing classes, and in some cases we can even remove the constructors, which shouldn't hurt; see https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-class-fields.md
It's probably possible to also change a lot of these class fields to private ones[2], however it's often difficult to tell at a glance if that's safe hence this patch only does this for the `PDFRenderingQueue`.
---
[1] This reduces the size of the `gulp mozcentral` output by 999 bytes, for a mostly mechanical code change.
[2] That sort of re-factoring should generally be done separately, on a class-by-class basis, to reduce the risk of regressions.
In all cases where we currently use `Response.prototype.arrayBuffer()` the result is immediately wrapped in a `Uint8Array`, which can be avoided by instead using the newer `Response.prototype.bytes()` method; see https://developer.mozilla.org/en-US/docs/Web/API/Response/bytes
Given that the Node.js code uses standard `ReadableStream`s now, see PR 20594, it can use the same `getArrayBuffer` as the Fetch API implementation.
Also, change the `getArrayBuffer` fallback case to an Error (rather than a warning) since that should never actually happen.
Currently the same identical code is duplicated four times per file, which seems completely unnecessary.
Note that the function isn't placed in `src/display/network_utils.js`, since that file isn't included in MOZCENTRAL builds.
Doing skip-cache reloading of https://opensource.adobe.com/dc-acrobat-sdk-docs/pdfstandards/PDF32000_2008.pdf#disableRange=true in the latest Firefox Nightly version I noticed an *intermittent* bug, where the loadingBar would fill up but without the PDF ever rendering.
Initially I was quite worried that the changes in PR 20602 had somehow caused this, however after a bunch of testing in a slightly older Nightly I was able to reproduce the problem there as well.[1]
Instead I believe that this problem goes all the back to PR 10675, so still my fault, since the `progressiveDone` handling there was incomplete.
Note how we only set the `this._done` property, but don't actually resolve any still pending requests. For reference, compare with the handling in the `PDFDataTransportStreamRangeReader.prototype._enqueue` method.
Depending on the exact order in which the `PDFDataTransportStreamReader.prototype.{_enqueue, read, progressiveDone}` methods are invoked, which depends on how/when the PDF data arrives, the bug might occur.
The reason that this bug hasn't been caught before now is likely that `disableRange=true` isn't used by default and Firefox users are unlikely to manually set that in e.g. `about:config`.
---
[1] Possibly some timings changed to make it slightly more common, but given the intermittent nature of this it's difficult to tell.
Report loading progress "automatically" when using the `PDFDataTransportStream` class, and remove the `PDFDataRangeTransport.prototype.onDataProgress` method
Currently this code expects a "url string", rather than a proper `URL` instance, which seems completely unnecessary now. The explanation for this is, as so often is the case, "historical reasons" since a lot of this code predates the general availability of `URL`.
This is consistent with the other `BasePDFStream` implementations, and simplifies the API surface of the `PDFDataRangeTransport` class (note the changes in the viewer).
Given that the `onDataProgress` method was changed to a no-op this won't affect third-party users, assuming there even are any since this code was written specifically for the Firefox PDF Viewer.
This should help reduce the maintenance burden of the code, since you no longer need to remember to update separate code when touching the different `DownloadManager` classes.
Given that we either use the `L10n` class directly or extend it via `GenericL10n`, it should no longer be necessary to keep the interface-definition.
This should help reduce the maintenance burden of the code, since you no longer need to remember to update separate code when touching the `L10n` class.
Given that `SimpleLinkService` now extends the regular `PDFLinkService` class, see PR 18013, it should no longer be necessary to keep the interface-definition.
This should help reduce the maintenance burden of the code, since you no longer need to remember to update separate code when touching the `PDFLinkService` class.
The percentage calculation is currently "spread out" across various viewer functionality, which we can avoid by having the API handle that instead.
Also, remove the `this.#lastProgress` special-case[1] and just register a "normal" `fullReader.onProgress` callback unconditionally. Once `headersReady` is resolved the callback can simply be removed when not needed, since the "worst" thing that could theoretically happen is that the loadingBar (in the viewer) updates sooner this way. In practice though, since `fullReader.read` cannot return data until `headersReady` is resolved, this change is not actually observable in the API.
---
[1] This was added in PR 8617, close to a decade ago, but it's not obvious to me that it was ever necessary to implement it that way.
Given that there's no less than *five* different, but very similar, implementations this helps reduce code duplication and simplifies maintenance.
Also, remove the `rangeChunkSize` not defined checks in all the relevant stream-constructor implementations.
Note how the API, since some time, always validates *and* provides that parameter when creating a `BasePDFStreamReader`-instance.
Given that there's no less than *five* different, but very similar, implementations this helps reduce code duplication and simplifies maintenance.
Also, spotted during rebasing, pass the `enableHWA` option "correctly" (i.e. as part of the existing `transportParams`) to the `WorkerTransport`-class to keep the constructor simpler.
Note how there's *nowhere* in the code-base where the `IPDFStreamRangeReader.prototype.onProgress` callback is actually being set and used, however the loadingBar (in the viewer) still works just fine since loading progress is already reported via:
- The `ChunkedStreamManager` instance respectively the `getPdfManager` function, through the use of a "DocProgress" message, on the worker-thread.
- A `IPDFStreamReader.prototype.onProgress` callback, on the main-thread.
Furthermore, it would definitely *not* be a good idea to add any `IPDFStreamRangeReader.prototype.onProgress` callbacks since they only include the `loaded`-property which would trigger the "indeterminate" loadingBar (in the viewer).
Looking briefly at the history of this code it's not clear, at least to me, when this became unused however it's probably close to a decade ago.
