[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #26100 [Applications/Tor Browser]: Update Tor Button for ESR 60
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs