PDFjs should stop using XHR.responseType="moz-chunked-arraybuffer"
Categories
(Firefox :: PDF Viewer, enhancement, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox68 | --- | fixed |
People
(Reporter: baku, Assigned: robwu)
References
Details
Attachments
(1 file, 1 obsolete file)
|
(deleted),
text/x-phabricator-request
|
Details |
This is a mozilla-only response type and we are planning to remove it. This seems to be used only when onProgressiveData is passed as argument in PdfJsNetwork.request(). But I don't see it used in our codebase. Can we maybe remove it easily? http://searchfox.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfJsNetwork.jsm#101-105
|
||
Comment 1•7 years ago
|
||
It's also used here https://github.com/mozilla/pdf.js/blob/master/src/display/network.js#L120 -- notice that the PDF.js library maybe be used my multiple document hosting vendors (e.g. OneDrive, DropBox, etc.) The PDF.js just implemented support for fetch() and will use it instead of moz-chunked-arraybuffer. I recommend to wait until bug 1389628 will reach the Firefox release channel and https://github.com/mozilla/pdf.js/pull/8768 reaches PDF.js release version (expected 1.10), to not regress to many people.
|
||
Updated•6 years ago
|
|
||
Comment 2•5 years ago
|
||
yury, are we ready to proceed here? I'm not sure what still needs to be done, given that PDF.js 2 has shipped with Firefox for a while (and so has ReadableStream).
|
|
||
Comment 3•5 years ago
|
||
:Snuffleupagus, would you happen to know if we still need -moz-chunked-arraybuffer for pdf.js in central? If not, maybe we can try to remove it.
| Comment hidden (obsolete) |
|
||
Comment 5•5 years ago
|
||
Looking at comment 3 again, I cannot help feeling that there's a few different questions being asked. Hence I'll obsolete my previous comment and try break this up into a hopefully more helpful response:
-
Can the platform support, i.e. https://searchfox.org/mozilla-central/rev/486b64e4bff86b7988af8c8b80845404ad197533/dom/webidl/XMLHttpRequest.webidl#25 and any related code, for
moz-chunked-arraybufferbe removed now?Yes, as far as I'm concerned. (I'd guess that this is the most important part of the question, from a wider Firefox perspective.)
-
Can the
moz-chunked-arraybuffersupport be removed from the Firefox-specific version of PDF.js now?Yes, because as far as I can tell that particular branch is actually dead code; please see the attached patch for additional information.
-
Can the
moz-chunked-arraybuffersupport be removed from the generic PDF.js library?No, not until both
FetchandReadableStreamsupport is available in Firefox ESR. However, as can be seen in https://github.com/mozilla/pdf.js/blob/bce9ff7347266e18326fff52c201faba04a96bd7/src/display/network.js#L56-L73 feature detection is used, hence I cannot see the harm in leaving this intact for the time being.
Unless I'm missing something here, this last point should be irrelevant for the platform removal ofmoz-chunked-arraybufferthough.
The above is my take, as an occasional PDF.js contributor, on the state of this bug.
Thomas, does this help clarify the current situation?
|
|
||
Comment 6•5 years ago
|
||
|
|
||
Comment 7•5 years ago
|
||
Ah, thank you for the details, that does clarify things.
I was asking as I'd like to try removing platform support for -moz-chunked-arraybuffer in bug 1120171, and this seemed to be the only obvious blocker I could find.
|
Assignee | |
Comment 8•5 years ago
|
||
Reviewing has moved off Bugzilla to Phabricator.
Jonas, can you and do you want to upload your patch to Phabricator? (see https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html)
If not, then I am willing to submit a patch to remove the unused code to get the ball rolling.
|
|
||
Comment 9•5 years ago
|
||
(In reply to Rob Wu [:robwu] from comment #8)
Reviewing has moved off Bugzilla to Phabricator.
I'm aware of that, and find it to be a very unfortunate decision.
Jonas, can you and do you want to upload your patch to Phabricator? (see https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html)
It seems that you need to enable 2FA in order to use Phabricator, and I don't have a device that I'm able to use for that purpose.
If not, then I am willing to submit a patch to remove the unused code to get the ball rolling.
It would be great if you could submit my patch from https://bugzilla.mozilla.org/show_bug.cgi?id=1411865#c6, since I'm not able to myself (unless Mozilla is willing to send me a 2FA device); thanks for offering to help!
|
|
Assignee | |
Comment 10•5 years ago
|
||
onProgressiveDatacallback is never set, so everything that is
conditional on it can be removed.loadedRequestshas never been used, it can be removed.- Several other methods are unused and not part of any interface;
These can also be removed.
|
|
||
Comment 11•5 years ago
|
||
Rob, shall I help get the patch here across the finish line, given Jonas cannot access Phabricator?
|
|
Assignee | |
Comment 12•5 years ago
|
||
I have already submitted the patch (with more cleanups besides Jonas' changes - see https://phabricator.services.mozilla.com/D24745).
The only blocker on this bug is a review from Brendan.
|
|
Assignee | |
Comment 13•5 years ago
|
||
Comment on attachment 9052823 [details] [diff] [review] bug1411865.patch Obsoleted by https://phabricator.services.mozilla.com/D24745
|
||
Comment 14•5 years ago
|
||
Pushed by rob@robwu.nl: https://hg.mozilla.org/integration/autoland/rev/1611d7c06c13 Remove dead code from PdfJsNetwork.jsm r=bdahl
|
||
Comment 15•5 years ago
|
||
| bugherder | ||
Description
•