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

Load file:-URLs in the main thread. #5100

Merged
merged 1 commit into from
Aug 6, 2014

Conversation

Rob--W
Copy link
Member

@Rob--W Rob--W commented Jul 29, 2014

QtWebKit does not support file:-URLs in Web Workers.
To solve this issue, I'm loading file:-URLs in the main thread for generic builds. file:-URLs load relatively quick, so there is no need for sophisticated load progress notifications.

Fixes #5057

@Rob--W
Copy link
Member Author

Rob--W commented Jul 30, 2014

@timvandermeij Could you take a look at this PR?
For background, see the test case at #5057 (comment) and the subsequent comment (#5057 (comment)).

@timvandermeij
Copy link
Contributor

I think this looks good, but I'll let someone else make the call for this one. One comment though: can we rename x to something more meaningful, i.e., request or something like that?

@Rob--W
Copy link
Member Author

Rob--W commented Jul 30, 2014

I've renamed x to xhr (which is consistent with similar code in src/core/network.js).

@yurydelendik
Copy link
Contributor

I guess we can take it. I think we need to minimize commented code for linting. Can you move xhr logic into e.g. file_utils.js and wrap into that function that will return Promise, for example readLocalFile(file).then(function () { open(); }, function() { error() });. (In the future we can move 'data' base64 decoding stuff there as well)

It makes sense to have PDFJS.getUrlProtocol(file) === 'file' instead of file && file.lastIndexOf('file:', 0) === 0 (I noticed we are using this operation a lot across the project, e.g. in utils.js or chromecom.js)

@Rob--W
Copy link
Member Author

Rob--W commented Jul 31, 2014

@yurydelendik I have removed the comments to allow linting. This snippet will be included in the GENERIC build only, which is the default, so it makes sense to have the code uncommented by default.

I haven't added getUrlProtocol because the uses in the viewer code (chromecom.js and this PR) are already obvious and very readable.

// very quickly, so there is no need to set up progress event listeners.
PDFView.setTitleUsingUrl(file);
var xhr = new XMLHttpRequest();
xhr.responseType = 'arraybuffer';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this line after xhr.open to avoid exceptions in Firefox?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

QtWebKit does not support file:-URLs in Web Workers.
To solve this issue, I'm loading file:-URLs in the main thread
for generic builds. file:-URLs load relatively quick, so there
is no need for sophisticated load progress notifications.
yurydelendik added a commit that referenced this pull request Aug 6, 2014
@yurydelendik yurydelendik merged commit 196416c into mozilla:master Aug 6, 2014
@yurydelendik
Copy link
Contributor

Thanks

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

Successfully merging this pull request may close these issues.

Unable to open default pdf in file:/// protocol when code embedded in qrc protocol (Qt)
3 participants