Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[api-minor] Remove the postMessageTransfers parameter, and thus the ability to manually disable transferring of data, from the API #11123

Merged
merged 3 commits into from
Sep 6, 2019
Merged

[api-minor] Remove the postMessageTransfers parameter, and thus the ability to manually disable transferring of data, from the API #11123

merged 3 commits into from
Sep 6, 2019

Commits on Sep 5, 2019

  1. Stop checking for the response property, on XMLHttpRequest, when … 

    …setting up the `WorkerMessageHandler`

This check was added in PR 2445, however it's no longer necessary since all data[1] is now loaded on the main-thread (and then transferred to the worker-thread).
Furthermore, by default the Fetch API is now (usually) used rather than `XMLHttpRequest`.

All in all, while these checks *were* necessary at one point that's no longer the case and they can thus be removed.

---
[1] This includes both the actual PDF data, as well as the CMap data.
    @Snuffleupagus
    Snuffleupagus committed Sep 5, 2019
    Configuration menu
    Copy the full SHA
    7212ff4 View commit details
    Browse the repository at this point in the history

  2. Adjust the values sent, with the 'test' message, by the `WorkerMessag… 

    …eHandler.setup` method

Note how the sent values have inconsistent types, with a boolean in one case and an object in the other (normal) case.
Furthermore, explicitly sending a `supportTypedArray: true` property seems superfluous at least to me.
    @Snuffleupagus
    Snuffleupagus committed Sep 5, 2019
    Configuration menu
    Copy the full SHA
    f0534b9 View commit details
    Browse the repository at this point in the history

  3. [api-minor] Remove the postMessageTransfers parameter, and thus the… 

    … ability to manually disable transferring of data, from the API

By transfering, rather than copying, `ArrayBuffer`s between the main- and worker-threads, you can avoid unnecessary allocations by only having *one* copy of the same data.
Hence manually setting `postMessageTransfers: false`, when calling `getDocument`, is a performance footgun[1] which will do nothing but waste memory.

Given that every reasonably modern browser supports `postMessage` transfers[2], I really don't see why it should be possible to force-disable this functionality.
Looking at the browser support, for `postMessage` transfers[2], it's highly unlikely that PDF.js is even usable in browsers without it. However, the feature testing of `postMessage` transfers is kept for the time being just to err on the safe side.

---
[1] This is somewhat similar to the, now removed, `disableWorker` parameter which also provided API users a much too simple way of reducing performance.

[2] See e.g. https://developer.mozilla.org/en-US/docs/Web/API/MessagePort/postMessage#Browser_compatibility and https://developer.mozilla.org/en-US/docs/Web/API/Transferable#Browser_compatibility
    @Snuffleupagus
    Snuffleupagus committed Sep 5, 2019
    Configuration menu
    Copy the full SHA
    7dea3f9 View commit details
    Browse the repository at this point in the history