[tbb-bugs] #26100 [Applications/Tor Browser]: Update Tor Button for ESR 60

[Tor Bug Tracker & Wiki]

#26100: Update Tor Button for ESR 60
-------------------------------------------------+-------------------------
 Reporter:  igt0                                 |          Owner:  tbb-
                                                 |  team
     Type:  defect                               |         Status:
                                                 |  needs_review
 Priority:  Very High                            |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  ff60-esr, tbb-torbutton,             |  Actual Points:
  TorBrowserTeam201805                           |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by arthuredelstein):

 * status:  needs_revision => needs_review


Comment:

 Here's a revised branch for review:
 https://github.com/arthuredelstein/torbutton/commits/26100+11
 (482abfb95de85bce98043338d8f7fcad9f6b7845)

 Details of the revision follow:

 Replying to [comment:8 gk]:
 > Okay, this works for me, nice! Here comes a first batch of review
 comments (sorry, I have to split this up as I must switch context but
 wanted you to give some feedback to already work on):
 >
 > commit 1eafafefd2dfb6006f9f2686e6ecb591da8bc434
 >
 > Adding a hint to the underlying Mozilla bug
 (https://bugzilla.mozilla.org/show_bug.cgi?id=1414390) to the commit
 message would be good I think.

 Done.

 > commit 629f58cfffefb3f58ceab7275e0f1e1194a6f012
 >
 > Is that new that `catch` statements can't have `if` statements anymore?

 Yes -- I added a link to the bugzilla bug.

 > commit 9c3e58597f3f3c41af2d44ae809a652c3a7d91a7
 >
 > Could you move the moz- removals in this bug to the previous commit?

 Done.

 > I don't understand your `plugin.disable` related commit/code change.
 It's not set by Mozilla code in ESR52 either. We are setting it in our own
 pref file we ship with the browser. Thus, it seems to me setting it here
 is not necessary?

 This was a leftover from mobile work and has been removed.

 > What speaks against using `getBrowser()`? It's cheaper to use
 `window.gBrowser`?

 igt0 noticed in base/content/browser.js:
 {{{
 function getBrowser() {
    return gBrowser;
 }
 /* DEPRECATED */
 }}}

 > Starting with "1." in the commit message without a "2." seems weird. :)
 Could you add more stuff as you seemed to want or reword that part?

 Reworded.

 > commit ec0e146668a817954ee75b78216151830e626ebc
 >
 > Please do a
 > {{{
 > const Cu = Components.utils;
 > }}}
 > and use `Cu` in `cookie-jar-selector.js`

 Done.

 > {{{
 > +  //Services.console.logStringMessage(`getPrefValue(${prefName})`);
 > }}}
 > No need for this commented out log statement

 Removed.

 > {{{
 > +       this.logmethod = 2;
 > }}}
 > Why is that needed?

 Removed.

 > There are two additional new lines in `startup-observer.js` which are
 not needed.

 Removed.

 > Re the pref loading:
 >
 > 1) Speaks anything against having that in utils.js instead of creating a
 new module?

 I'm inclined to have a separate module because, unlike the function in
 utils.js, this function uniquely needs to run before most other code. But
 I don't feel too strongly.

 > 2) I think we should set the prefs on the default prefs branch as it is
 done right now. That allows the user easily see which preferences got
 modified by themselves/the browser and more importantly there is the
 option to return to save defaults. tor-launcher's
 949379555010a04633c64b08ed52896a86c2cce6 might give some inspiration.

 Good point! I think I have fixed this.

 Replying to [comment:9 gk]:
 [snip]
 > c1f2c1f79d334c43ffdbd598312d5de2e3b54a34
 >
 > "tor button" -> "Torbutton"
 [snip]

 Fixed.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/26100#comment:13>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online