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

Fix disableAutoFetch regression in the generic viewer #5371

Merged
merged 1 commit into from
Oct 1, 2014
Merged

Fix disableAutoFetch regression in the generic viewer #5371

merged 1 commit into from
Oct 1, 2014

Conversation

Snuffleupagus
Copy link
Collaborator

After PR #5263, setting disableAutoFetch = true no longer works correctly (the entire file loads). This is a tentative patch that attempts to address that.

@yurydelendik
Copy link
Contributor

hmm, that's not really a regression by design (to enable incomplete file loading both disableStream=true and disableAutoFetch=true must be set), since enabling stream shall automatically disableAutoFetch. we have a confusion here with the options and this PR might be an acceptable solution. however it's better to see something like enumeration autoFetch=[disabled|stream|range|auto]:

  • disable: disableAutoFetch=true, disableStream=true;
  • stream: disableAutoFetch=true, disableStream=false;
  • range: disableAutoFetch=false, disableStream=true;
  • auto: disableAutoFetch=false, disableStream=false; (if stream is not available, falls to auto-fetch)

@yurydelendik
Copy link
Contributor

Just thinking: if we let disableAutoFetch to disable stream, there is really no need in disableStream

@Snuffleupagus
Copy link
Collaborator Author

hmm, that's not really a regression by design (to enable incomplete file loading both disableStream=true and disableAutoFetch=true must be set),

A valid point, that I completely overlooked, sorry about that!
This works well in the addon given the code in PdfStreamConverter.jsm, but not in the generic viewer (where it is a regression, in my opinion).
@yurydelendik Any objection against me re-purposing this PR to just fix the generic viewer, and then we can explore your enumeration idea in a followup if we want to?

@yurydelendik
Copy link
Contributor

I see. Yeah we need to fix "disableStream=true and disableAutoFetch=true" thing for the generic viewer

@Snuffleupagus Snuffleupagus changed the title Fix disableAutoFetch regression Fix disableAutoFetch regression in the generic viewer Oct 1, 2014
After PR 5263, setting `disableAutoFetch = true` in the generic viewer no longer works correctly, since the entire file loads even with `disableStream = true`.
@yurydelendik
Copy link
Contributor

/botio-windows preview

@pdfjsbot
Copy link

pdfjsbot commented Oct 1, 2014

From: Bot.io (Windows)


Received

Command cmd_preview from @yurydelendik received. Current queue size: 0

Live output at: http://107.22.172.223:8877/b2fd88de096a903/output.txt

yurydelendik added a commit that referenced this pull request Oct 1, 2014
Fix disableAutoFetch regression in the generic viewer
@yurydelendik yurydelendik merged commit dc30dba into mozilla:master Oct 1, 2014
@Snuffleupagus Snuffleupagus deleted the disableAutoFetch-regression branch October 1, 2014 21:51
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 this pull request may close these issues.

3 participants