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

Remove the "useOnlyCssZoom" (debugging) hash parameter #11533

Merged
merged 2 commits into from
Jan 23, 2020

Conversation

Snuffleupagus
Copy link
Collaborator

The debugging hash parameters[1] are intended to facilitate access to various tools/settings in PRODUCTION builds, protected by the pdfBugEnabled preference.
At this point, the remaining debugging hash parameters are mainly intended to allow access to the PDFBug tools and/or to quickly toggle certain larger features.

The "useOnlyCssZoom" functionality doesn't really seem to fit in with the rest of these hash parameters, since:

  • This is, comparatively speaking, a minor viewer-specific feature.
  • The zooming implementation will (almost) always fallback to CSS-only zooming, for any document, once the canvases becomes large enough. Hence, the majority of the CSS zooming feature can still be tested directly in any build of the viewer.
  • After the initial implementation, years ago, the CSS-only zooming code in question hasn't changed much (or even at all), i.e. it doesn't seem like an active development target.[2]
  • If the "useOnlyCssZoom" functionality was added today, it's unlikely that a hash parameter would've been added.
  • Last, but not least, there's also a useOnlyCssZoom preference hence toggling this functionality shouldn't be too difficult (e.g. if someone needs to hack on it).

All in all, I'm thus suggesting that we remove the "useOnlyCssZoom" hash parameter.


[1] Originally these hash parameters could be used directly in any build, which was bad since it would allow any link to potentially disable functionality and/or reduce performance.

[2] If it had seen active development over the years, I'd be much more inclined to keep the hash parameter.

…hash is empty

For people running e.g. Firefox with the `pdfBugEnabled` preference set, to allow quick access to debugging tools, this method will obviously run for every opened PDF file. However, in most cases the URL hash is empty and we can thus skip most of the parsing and simply return early instead.
The debugging hash parameters[1] are intended to facilitate access to various tools/settings in PRODUCTION builds, protected by the `pdfBugEnabled` preference.
At this point, the remaining debugging hash parameters are mainly intended to allow access to the `PDFBug` tools and/or to quickly toggle certain larger features.

The "useOnlyCssZoom" functionality doesn't really seem to fit in with the rest of these hash parameters, since:
 - This is, comparatively speaking, a minor viewer-specific feature.
 - The zooming implementation will (almost) always fallback to CSS-only zooming, for any document, once the canvases becomes large enough. Hence, the majority of the CSS zooming feature can still be tested *directly* in any build of the viewer.
 - After the initial implementation, years ago, the CSS-only zooming code in question hasn't changed much (or even at all), i.e. it doesn't seem like an active development target.[2]
 - If the "useOnlyCssZoom" functionality was added today, it's unlikely that a hash parameter would've been added.
 - Last, but not least, there's also a `useOnlyCssZoom` preference hence toggling this functionality shouldn't be too difficult (e.g. if someone needs to hack on it).

All in all, I'm thus suggesting that we remove the "useOnlyCssZoom" hash parameter.

---
[1] Originally these hash parameters could be used directly in any build, which was bad since it would allow any link to potentially disable functionality and/or reduce performance.

[2] If it had seen active development over the years, I'd be *much* more inclined to keep the hash parameter.
@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/280cfe04135f6a0/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/280cfe04135f6a0/output.txt

Total script time: 1.73 mins

Published

@timvandermeij timvandermeij merged commit 9791d7e into mozilla:master Jan 23, 2020
@timvandermeij
Copy link
Contributor

Good idea to clean up the hash parameters a bit. Thanks!

@Snuffleupagus Snuffleupagus deleted the rm-useOnlyCssZoom-hash branch January 23, 2020 23:49
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