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.

Sign up for GitHub

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

Jump to bottom

Compatibility: disable range requests for iOS and refactor user agent detection #7841

Merged
merged 2 commits into from
Nov 22, 2016
Merged

Compatibility: disable range requests for iOS and refactor user agent detection #7841

merged 2 commits into from
Nov 22, 2016

Conversation

timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Nov 22, 2016 •
edited
Loading

Fixes #7815.
Supersedes #7822.
Supersedes #7824.

@timvandermeij
Compatibility: refactor user agent detection
cf3a742 
This patch moves the user agent checks to the top of the file to reduce
duplication and to provide a clear overview of which user agent we are
detecting.

Moreover, we extract inline user agent checks as well and use existing
checks in more places. Finally, we fix the indenting in one place for
consistency.
@Snuffleupagus Snuffleupagus merged commit ef7fd75 into mozilla:master Nov 22, 2016
@Snuffleupagus
Copy link
Collaborator

Thanks for the patch!

@timvandermeij timvandermeij deleted the ios-range branch November 22, 2016 21:57
This was referenced Nov 22, 2016
Closed
Closed
@websirnik
Copy link

@timvandermeij I had range requests working on iOS10/9 . I'm not sure it is a right approach to disable them completely for the iOS. Though I did experience the same issue as @Snuffleupagus in WeChat and the way I've fixed it was similar to #7822 . Targeted conditions are better in this case.

@Snuffleupagus
Copy link
Collaborator

I'm not sure it is a right approach to disable them completely for the iOS.

Perhaps not, but it's a simple one and we really don't want to waste a lot of development/testing time on fine-tuning compatibility.js. Its purpose is to make it possible to use (most) browsers that have missing/broken features together with PDF.js out of the box, but you're certainly free to not use compatibility.js, or to modify it to suite your needs, in custom deployments.
Generally speaking we will err on the side of caution, and disable some features perhaps unnecessarily for certain browsers (for simplicity), since more fine-grained feature testing is often difficult.

Though I did experience the same issue as @Snuffleupagus in WeChat and the way I've fixed it was similar to #7822 .

Please note that I've never used, nor have I claimed to use, the WeChat browser (which seems to be specific to the Chinese market).

@websirnik
Copy link

Sorry, meant to mention the other user who had WeChat issue.

I see where you stand on this issue. Being on the cautionary side makes sense. Someone might argue that fine-tuning compatibility.js for quite a large audience of iOS users might make more sense.
I personally ok to locally fine-tune compatibility.js in my deployment.

@Snuffleupagus Snuffleupagus mentioned this pull request Jan 9, 2017
Closed
@dennybiasiolli dennybiasiolli mentioned this pull request Jan 17, 2017
Closed
movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
@Snuffleupagus
Merge pull request mozilla#7841 from timvandermeij/ios-range
4477274 
Compatibility: disable range requests for iOS and refactor user agent detection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Snuffleupagus Snuffleupagus Snuffleupagus approved these changes

Assignees
No one assigned
Labels
viewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@timvandermeij @Snuffleupagus @websirnik