Skip to content

Replace the IPDFStream, IPDFStreamReader, and IPDFStreamRangeReader interfaces with proper base classes#20602

Merged
timvandermeij merged 8 commits intomozilla:masterfrom
Snuffleupagus:BasePDFStream-2
Feb 1, 2026
Merged

Replace the IPDFStream, IPDFStreamReader, and IPDFStreamRangeReader interfaces with proper base classes#20602
timvandermeij merged 8 commits intomozilla:masterfrom
Snuffleupagus:BasePDFStream-2

Conversation

@Snuffleupagus
Copy link
Collaborator

No description provided.

@Snuffleupagus
Copy link
Collaborator Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/9d479a12e1db3a9/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/bd343bca84232ef/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/bd343bca84232ef/output.txt

Total script time: 42.08 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 1

Image differences available at: http://54.241.84.105:8877/bd343bca84232ef/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/9d479a12e1db3a9/output.txt

Total script time: 82.70 mins

  • Unit tests: FAILED
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 1

Image differences available at: http://54.193.163.58:8877/9d479a12e1db3a9/reftest-analyzer.html#web=eq.log

…lass

Currently this property is essentially "duplicated", so let's instead use the identical one that's availble on the `ChunkedStream` instance.
…ed` getter

This getter was only invoked from `src/display/network.js` and `src/core/chunked_stream.js`, however in both cases it's hardcoded to `false` and thus isn't actually needed.
This originated in PR 6879, close to a decade ago, for a potential TODO which was never implemented and it ought to be OK to just simplify this now.
…eReader`-instance, in the `ChunkedStreamManager` class

Given that nothing in the `PDFWorkerStreamRangeReader` class attempts to invoke the `onProgress` callback, this is effectively dead code now.
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.

Finally, note also how progress is already being reported through the `ChunkedStreamManager.prototype.onReceiveData` method.
…erface

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.
…implementations inherit from

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.
…reamReader` implementations inherit from

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.
…PDFStreamRangeReader` implementations inherit from

Given that there's no less than *five* different, but very similar, implementations this helps reduce code duplication and simplifies maintenance.
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.
@timvandermeij timvandermeij merged commit e4cd317 into mozilla:master Feb 1, 2026
11 checks passed
@timvandermeij
Copy link
Contributor

Thank you for refactoring this!

@Snuffleupagus Snuffleupagus deleted the BasePDFStream-2 branch February 1, 2026 16:45
@Snuffleupagus
Copy link
Collaborator Author

Thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants