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.

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

Implement Fetch API for streaming PDF loads on Chrome #6126

Closed
jordan-thoms opened this issue Jun 18, 2015 · 9 comments · Fixed by #8768
Closed

Implement Fetch API for streaming PDF loads on Chrome #6126

jordan-thoms opened this issue Jun 18, 2015 · 9 comments · Fixed by #8768
Labels

Comments

@jordan-thoms
Copy link
Contributor

Chrome has now shipped the Fetch API ( https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/35_QSL1ABTY ) in Chrome 43. I'm not sure yet if other browser vendors will implement it, but seeing as PDF.js has support for the firefox-only moz-chunk-arraybuffer, it seems reasonable to implement this for Chrome. We often see poor network performance loading PDFs, especially when using range requests, and hopefully this can help.

I'm planning to start working on this over the next few days, any initial thoughts?

@jordan-thoms
Copy link
Contributor Author

Actually to correct that, seems it's already implemented in Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1039846

@Snuffleupagus
Copy link
Collaborator

Is this perhaps a duplicate of issue #5319?

@jordan-thoms
Copy link
Contributor Author

Yes I guess so. It appears that it's also possible to do streaming through the normal XHR api, so I'll try that first since it will most likely require less code changes.

EDIT: Actually, seems there are substantial advantages to doing this through the Fetch API so that seems preferable.

@timvandermeij
Copy link
Contributor

See #6348 for additional information. If someone is willing to work on this, feel free to submit a pull request. We must be sure to remain compatible for old browsers, so we probably need a fallback to XHR or a polyfill for fetch() to do so.

@nschloe
Copy link
Contributor

nschloe commented Aug 12, 2015

FYI, GitHub published a fetch polyfill.

@yurydelendik
Copy link
Contributor

Our main use case is progressive fetching portions of the remote file. The solution using Fetch API shall not regress our streaming and range fetching capabilities.

@yurydelendik
Copy link
Contributor

We use sync-XHR in l10n and cmap components. We shall probably create (blocking?) issues to migrate both to async-XHR/fetch as well.

@Rob--W
Copy link
Member

Rob--W commented Aug 13, 2015

@yurydelendik I patched l10n to not use sync XHR a while back (#5739), so only cmaps and fonts need to be fixed.

Note that fetch is not yet usable as a full replacement for XHR, because:

  • Chrome: Permissions whitelists (e.g. extension permissions, or --disable-web-security flag) does not propagate to the fetch API https://crbug.com/478345 (fixed as of 45.0.2402.0).
  • Chrome: Anything other than http: and https: is ignored, including blob:, data:, file:, filesystem: and ftp: URLs. https://crbug.com/455109 (no fix yet)
  • Design flaw: fetch requests cannot be cancelled (like XHR's abort). There is no resolution yet, the discussion seems to have stalled due to bikeshedding (Aborting a fetch whatwg/fetch#27)
  • Firefox: fetch requests are not aborted upon unload: https://bugzil.la/1165237
  • Limitations of polyfills: fetch cannot be implemented in terms of XHR (fetch is way more powerful), and XHR cannot be implemented in terms of fetch (e.g. missing .abort()). We should try to get the best of both APIs, and not add an incomplete polyfill that is only an incomplete shim of both APIs).

In theory, fetch supersedes XMLHttpRequest, and fetch would replace XHR, but in practice that's not possible yet. fetch's only advantage over XHR (from PDF.js's perspective) is streaming response bodies. Given the above limitations, if we want to use fetch, we still have to keep the XHR implementation in the core.

And, note that fetch support does not imply support for readable body streams. Chrome supported fetch since 42, but streams were only supported since 43.

@yurydelendik
Copy link
Contributor

Thanks, @Rob--W

Filed #6352 for async cmaps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants