[tor-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_revision
 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 gk):

 * status:  needs_review => needs_revision


Comment:

 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.


 commit 629f58cfffefb3f58ceab7275e0f1e1194a6f012

 Is that new that `catch` statements can't have `if` statements anymore?


 commit 9c3e58597f3f3c41af2d44ae809a652c3a7d91a7

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

 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?

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

 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?


 commit ec0e146668a817954ee75b78216151830e626ebc

 Please do a
 {{{
 const Cu = Components.utils;
 }}}
 and use `Cu` in `cookie-jar-selector.js`
 {{{
 +  //Services.console.logStringMessage(`getPrefValue(${prefName})`);
 }}}
 No need for this commented out log statement
 {{{
 +         this.logmethod = 2;
 }}}
 Why is that needed?

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

 Re the pref loading:

 1) Speaks anything against having that in utils.js instead of creating a
 new module?

 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.

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