Use Content-Range instead of Content-Length #5512 #5603
Conversation
|
this would actually make pdf.js more spec compliant? |
|
@Rob--W I think we can take it. Could you add preprocessor |
Rob--W
force-pushed
the
xhr-range-206-bugfix
branch
from
386c67e to
b752764
Compare
|
@CodingFabian @yurydelendik Done! |
|
well if it is a bug fix, then it should be in compatibility.js? |
|
@CodingFabian yes, for this one we can make temporary exception. I think we can still use #5598 after it Chrome fix reaches release and we can remove this code. |
|
i do not understand. why is @Rob--W fix now better, but later mine can be taken? I am fine with either solution, but want to understand why you prefer one over the other. |
|
@CodingFabian My "fix" is to detect when the browser misbehaves and recover from this error. The advantage of this method over yours is that it has no unwanted side-effects, and that the performance benefits of range requests are still available. |
|
well actually I am waiting for over a month now for a fix. The moment I decided to apply a clean solution, you come around with your patch which violates the separation of concerns in code. To repeat the current situation: Since a month all Chrome 39+ browsers do not display a PDF at all. Your argument is that a solution which at least will make PDFs render for 30% browser Market share is not optimal, instead you come up with a solution which might work around the issue, but is actually not wanted as permanent solution. Again: For Safari and Android there is already a range disable in compatibility, why are you objecting against adding one for chrome? |
|
@CodingFabian I was going to take yours, but from the #5512 I understood we still looking for better workaround (which this fits to the description :). Yours is good for long term compability, however you did not set upper version limit yet, which means it's still incomplete and need a follow up work later. This one is Chrome specific (really bad ihmo) however is more precise and I consider this as a temporary (to replace with yours). I have no preference since not using Chrome in everyday life. For Safari and Android, we can take a simple workaround that does not adds any extra headers into request or performs any long running feature tests (to avoid toll on other good browsers). I still don't understand what is the issue there. |
|
@yurydelendik you call. i am happy when there is a build that contains any fix so we can activate range requests again. |
|
@yurydelendik Could you merge this PR? I want to publish an update of the Chrome extension because users are affected by it. |
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 0 Live output at: http://107.22.172.223:8877/89c5ed9f66b824d/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 0 Live output at: http://107.21.233.14:8877/18c243f321b8915/output.txt |
| @@ -106,6 +106,22 @@ var WorkerMessageHandler = PDFJS.WorkerMessageHandler = { | |||
|
|
|||
| var length = fullRequestXhr.getResponseHeader('Content-Length'); | |||
| length = parseInt(length, 10); | |||
| //#if !(FIREFOX || MOZCENTRAL) | |||
There was a problem hiding this comment.
NIt: since the code shouldn't be necessary in FirefoxOS either, this line should ideally be: //#if !(FIREFOX || MOZCENTRAL || B2G).
There was a problem hiding this comment.
Wouldn't it make more sense to use (GENERIC || CHROME) instead? That'd be more explicit than listing everything that should not be included.
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/89c5ed9f66b824d/output.txt Total script time: 17.62 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/18c243f321b8915/output.txt Total script time: 23.18 mins
|
Use Content-Range header instead of Content-Length when the response has status code 206, to work around issue mozilla#5512, which is caused by a bug in Chrome (since version 39): https://code.google.com/p/chromium/issues/detail?id=442318
Rob--W
force-pushed
the
xhr-range-206-bugfix
branch
from
b752764 to
c02b2cb
Compare
|
Amended commit: Changed |
|
Good, thank you. |
Use Content-Range instead of Content-Length #5512
Use Content-Range instead of Content-Length mozilla#5512
Revert #5603 regarding Chrome range request bug
* 'master' of https://github.com/mozilla/pdf.js: (51 commits) Revert mozilla#5603 regarding Chrome range request bug Disable Range Support for Chrome 39+40 (mozilla#5512) Prevent Firefox from warning about |unreachable code after return statement| Simplify document properties field logic Enable linting of Firefox specific code in viewer.js Group public/private methods and add comments Clarify bug reporting with regards to providing a pdf Add a reduced test-case for issue 5801 Attempt to infer if a CMap file actually contains just a standard `Identity-H`/`Identity-V` map Refactor PDFDocumentProperties to be more class-like Rename DocumentProperties to PDFDocumentProperties Remove no longer needed hacks that enable spacebar scrolling in Firefox (issue 3498) Preface all "fullscreen" CSS rules with a |pdfPresentationMode| class, and add it to the |viewerContainer| while Presentation Mode is active Refactor PDFPresentationMode to be more class-like Re-ordering the PDFPresentationMode code so that the "public" functions are placed towards the top of the file Initial refactoring of the PDFPresentationMode code Rename the presentation_mode.js file and adjust the function names Refactor the options passed to |PresentationMode.initialize| and clean-up some code in viewer.js and presentation_mode.js Move the PresentationMode-specific scrollWheel code from PDFViewerApplication Break dependencies between PresentationMode and other code, and add PresentationMode related utility methods to PDFViewer ...
Use Content-Range header instead of Content-Length when the response has status code 206, to work around issue #5512, which is caused by a bug in Chrome (since version 39): https://code.google.com/p/chromium/issues/detail?id=442318