[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #19459 [Applications/Tor Browser]: Write (C++) patch for window resizing parts

#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@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs