Updated zoom constants and pages to resolve issue #6943

[fow5040]

Fix brave/brave-browser#6943

Submitter Checklist:

• Submitted a ticket for my issue if one did not already exist. - see Add more zoom levels, like 133% brave-browser#6943
• Used Github auto-closing keywords in the commit message.
• Added/updated tests for this change (for new code or code which already has tests).
• Verified that these changes build without errors on
  • Android
  • iOS
  • Linux
  • macOS
  • Windows
• Verified that these changes pass automated tests (unit, browser, security tests) on
  • iOS
  • Linux
  • macOS
  • Windows
• Verified that all lint errors/warnings are resolved (npm run lint)
• Ran git rebase master (if needed).
• Ran git rebase -i to squash commits (if needed).
• Tagged reviewers and labelled the pull request as needed.
• Request a security/privacy review as needed.
• Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone
• Public documentation has been updated as necessary. For instance:
  • https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
  • https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
  • https://github.com/brave/brave-browser/wiki/Fingerprinting-Protection-Mode
  • https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
  • https://github.com/brave/brave-browser/wiki/Custom-Headers
  • https://github.com/brave/brave-browser/wiki/Web-compatibility-issues-with-tracking-protection
  • https://github.com/brave/brave-browser/wiki/QA-Guide

Test Plan:

1. Verify you can set Zoom level in brave://settings to 133% and 140%
2. Open a PDF
3. Verify you can use Zoom level 133% and 140% (icon shows in omnibar)
4. Try to print the page
5. Print preview will come up
6. Verify you can zoom to 133% and 140%
   • Zoom controls are on the left side (- and +)
   • To check the zoom value, you need to right click the preview and pick Inspect
   • Once the debugger comes up, go to the console
   • In the console, type in window.viewer.viewport_.internalZoom_
   • Zoom % value should show

Reviewer Checklist:

• New files have MPL-2.0 license header.
• Request a security/privacy review as needed.
• Adequate test coverage exists to prevent regressions
• Verify test plan is specified in PR before merging to source

After-merge Checklist:

• The associated issue milestone is set to the smallest version that the
  changes has landed on.
• All relevant documentation has been updated.

[bsclifton]

Very nice! I think we can do this without patches (we have a mechanism that overrides the files at the GN level). Let me see if I can re-work your PR... 😄

[fow5040]

Ahh, thank you, got it! Just for my reference, is it the subclass/override method listed in the patching chromium wiki, or something else entirely?

[bsclifton]

@fow5040 I thought it would be a slam dunk, but it's actually a little trickier...

That wiki entry is exactly what would work for the C/C++ ones 😄I made an edit which does that here: e3b534c

The JavaScript ones are a bit tougher; we do have a template system in which you can define a "behavior" which would then (at runtime) replace the array. I have this working... but only if I change the array to be readonly=false. Here's the settings override JavaScript we have (which gets ran on Polymer pages):
https://github.com/brave/brave-core/blob/master/browser/resources/settings/brave_settings_overrides.js

My edit (so far) looks like this:

diff --git a/browser/resources/settings/brave_settings_overrides.js b/browser/resources/settings/brave_settings_overrides.js
index c55966d30..7a4ab6f2d 100644
--- a/browser/resources/settings/brave_settings_overrides.js
+++ b/browser/resources/settings/brave_settings_overrides.js
@@ -103,8 +103,43 @@ const BraveClearSettingsMenuHighlightBehavior = {
   }
 }

+const BraveAddZoomLevelsBehavior = {
+  ready: function() {
+    console.log('BSC]] START ', this.pageZoomLevels_)
+    this.pageZoomLevels_Handler = function(levels) {
+      this._setPageZoomLevels_(levels);
+    }
+
+    this.pageZoomLevels_ = [
+        1 / 4,
+        1 / 3,
+        1 / 2,
+        2 / 3,
+        3 / 4,
+        4 / 5,
+        9 / 10,
+        1,
+        11 / 10,
+        5 / 4,
+        4 / 3,
+        7 / 5,
+        3 / 2,
+        7 / 4,
+        2,
+        5 / 2,
+        3,
+        4,
+        5,
+      ]
+    console.log('BSC]] END')
+  }
+}
+
 // Polymer Component Behavior injection (like superclasses)
 BravePatching.RegisterPolymerComponentBehaviors({
+  'settings-appearance-page': [
+    BraveAddZoomLevelsBehavior
+  ],
   'settings-clear-browsing-data-dialog': [
     BraveClearBrowsingDataOnExitBehavior
   ],

[bsclifton]

Here we go- second one fixed and working 😄 589db3f

The last one I was talking through how to make the fix with @bridiver. It'll be the hardest, as we don't have any examples (currently)

[fow5040]

Got it! Thanks for updating this thread with examples, will keep it in mind for any potential future contributions. :)

[bsclifton]

You're very welcome! Thanks for taking the time to fix this, it's greatly appreciated 😄

We try to be more careful about patching (as you saw in the guide) to help keep rebases (when new Chromium versions come out) much easier. Will help with this last example (it's a little trickier) then we can accept this! 😎

This fix would be a great one to upstream, honestly- I could help you push it to Chromium if you were interested. Let me know 😄

[fow5040]

Absolutely - will keep these changes in mind (including the fancy viewport .js overrides). If you think it'd be a valuable change to merge with Chromium I'd be happy to put it there as well!

As far as I'm concerned I just did a little bit of digging and some minor tweaking, but always happy to push it if it'll be useful to the user base at large.

Thanks again for all the guidance 🙌

[bsclifton]

@fow5040 with the help of @simonhong I was able to solve the last one 😄 No more patches! Please take a look; you should be able to pull this PR down now and verify it works for you

BTW - I added a test plan that covers the 3 areas called out in the source

@bridiver this is now ready for review 😄👍

[bsclifton]

and whoops; a little too quick there on my part (doh). I loaded up print preview and noticed this error:

[20247:775:1128/003811.688271:ERROR:CONSOLE(6)] "Uncaught SyntaxError: Cannot use import statement outside a module", source: chrome://print/pdf/viewport.js (6)
[20247:775:1128/003811.799828:ERROR:CONSOLE(187)] "Uncaught (in promise) ReferenceError: Viewport is not defined", source: chrome://print/pdf/pdf_viewer.js (187)

It doesn't appear that the pre-processing is happening for this file (the import should be getting replaced with the actual content)

[bsclifton]

Fix found! Thanks @simonhong

OK - @bridiver, this is ready for review 😄

[bsclifton]

LGTM 👍 Tested on macOS

[bsclifton]

and merged! Thanks for the PR, @fow5040 😄