#19459: Write (C++) patch for window resizing parts -------------------------------------------------+------------------------- Reporter: gk | Owner: | arthuredelstein Type: task | Status: | needs_review Priority: Medium | Milestone: Component: Applications/Tor Browser | Version: Severity: Normal | Resolution: Keywords: tbb-torbutton-conversion, | Actual Points: TorBrowserTeam201610R | Parent ID: | Points: Reviewer: | Sponsor: | SponsorU -------------------------------------------------+------------------------- Changes (by arthuredelstein):
* keywords: tbb-torbutton-conversion, TorBrowserTeam201610 => tbb- torbutton-conversion, TorBrowserTeam201610R * status: needs_revision => needs_review Comment: Thanks, gk, mcs and brade for your reviews! Replying to [comment:35 gk]: > It does, neat! One nit: could you put a comment above the JS code change explaining what is going on? (maybe pointing to #18175 additionally?) Good idea -- added. Replying to [comment:36 mcs]: > Kathy and I reviewed both patches. Here are a few comments on the Torbutton patch: > * In `torbutton_resizelistener.onStateChange`, the container variable can be removed. Done. > * The `extensions.torbutton.startup_resize_period` pref (aka `k_tb_tor_resize_warn_pref`) is never set to false and its value is never read, so it seems like it can be removed too. Removed. > * Since the call to `quantizeBrowserSize()` was removed, should the entire `src/chrome/content/content-sizer.js` file be removed? Or will that functionality be reinstated? Oops, I restored that function call. > And here are our comments on the browser (C++) patch: > * Please add a brief comment above `nsXULWindow::ResizeToRoundedDimensions()` to explain what it does (1000 x 1000 maximum, width constrained to a multiple of 200, etc.) Added. > * `nsXULWindow::ResizeToRoundedDimensions()` should have `return NS_OK` at the end. Fixed. > * Are we sure that it is okay to remove support for the `extensions.torbutton.window.innerWidth` / `innerHeight` prefs? With the new patches, there is no way for the user to override the initial window size unless they disable fingerprinting protection. That might be okay, but I think people who have set these prefs in the past will be surprised. I have added two prefs: privacy.window.maxInnerWidth and privacy.window.maxInnerHeight. Hopefully these will be enough to solve any such problems. > A related issue (but not really for this ticket) is that constraining the width to a multiple of 200 might be annoying on Android devices that have narrow screens (old phones)? Good point. We could consider allowing smaller quantizations for small window sizes. Here's the new version: https://github.com/arthuredelstein/tor-browser/commit/19459+14 https://github.com/arthuredelstein/torbutton/commit/19459+1 -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/19459#comment:39> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online _______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs