[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #25750 [Applications/Tor Launcher]: update Tor Launcher for ESR 60
#25750: update Tor Launcher for ESR 60
--------------------------------------------+------------------------------
Reporter: mcs | Owner: brade
Type: defect | Status: needs_review
Priority: Very High | Milestone:
Component: Applications/Tor Launcher | Version:
Severity: Normal | Resolution:
Keywords: ff60-esr, TorBrowserTeam201805 | Actual Points:
Parent ID: | Points:
Reviewer: | Sponsor:
--------------------------------------------+------------------------------
Changes (by sysrqb):
* status: needs_revision => needs_review
Comment:
Replying to [comment:15 mcs]:
> Replying to [comment:9 sysrqb]:
> > I'll set this to needs-review for the tor-launcher patches I have:
> > Branch bug25750 on git.tp.o/user/sysrqb/tor-launcher.git
>
> Kathy and I reviewed and ran the revised Tor Launcher in a ESR 60-ish
browser. It is working pretty well – good work! We have a few comments:
Thanks!
>
> 1) I am surprised that XPCOM extensions still work as well as they do.
We should set `extensions.legacy.enabled = true` and add `tor-
launcher@xxxxxxxxxxxxxx` to `extensions.legacy.exceptions` so the
about:addons UI does not label Tor Launcher as LEGACY (and also add
Torbutton's extension ID if we keep them both as regular add-ons).
Yes, I was surprised, too. I think FF61 brings a lot more breakage - but I
think these should be set in/by tor-browser instead of tor-launcher.
>
> 2) For 5e5fba0951b0837948bd9c074a3710afe137d0eb, please omit the first
change.
Done.
>
> 3) For 6105ea64e74cf65e09755d917234fe63b49b84ba, you could use a shorter
commit message, e.g.,
> Mozilla dropped support for Ci.nsIProgrammingLanguage.JAVASCRIPT
> See https://bugzilla.mozilla.org/show_bug.cgi?id=1149830#c18
Done.
>
> 4) For 068b9b5e8bb0408879b984c7c7ebd3726f25dd6c you could shorten the
commit message and reference the entire bugzilla.mozilla.org URL. Maybe
just use:
> Gecko now requries "0o"-prefixed octal literals
> See https://bugzilla.mozilla.org/show_bug.cgi?id=1263074
>
Done.
> 5) For 374b64f3057ab4b703e3e79473cd4cfc8abc295e:
> * How can we guarantee that the call to `_loadPreferences()` occurs
before any code which might rely on the preference values? Kathy and I
don't have an answer, but maybe we should add an explicit initialization
call inside `components/tl-process.js` (either in the `"profile-after-
change"` case within the `observe()` implementation or at the end of `tl-
process.js`.
This was a little tricky because torlauncher uses preferences very early
in its initialization (such as initializing the TorLauncherLogger module).
As a result, I moved loading the prefs into the TorProcessService
constructor. In addition, now an exception is thrown if certain methods
are called before we load the default prefs (get{Bool,Int,Char}Pref(),
TorStartAndControlTor(), etc).
I think we want Tor Browser to fail closed in these situations. Further, I
think we want Tor Browser failing closed if any of the TorLauncher default
prefs are invalid or aren't set for any reason.
> * Please use `let` instead of `var` in new code when possible (we are
migrating in that direction).
> * Please use the Cc and Ci constants.
> * Please log a message when ignoring invalid pref names (near the top of
the `pref()` function).
All done.
> * You can use `this.mPrefsSvc` inside `_getPrefDefaultBranch()`; if you
do that, it might not be worthwhile to have a `_getPrefDefaultBranch()`
function.
I don't see how we can access the DefaultBranch from `this.mPrefsSvc`
because mPrefsSvc is a `Ci.nsIPrefBranch` but we need `Ci.nsIPrefService`
for retrieving the default branch. If there's a way to get the default
branch from nsIPrefBranch, then I'll do that.
One option, which requires more changes, is changing `mPrefsSvc` a
`nsIPrefBranch`. Then we can use both `getBranch()` and
`getDefaultBranch()` from that.
https://developer.mozilla.org/en-US/docs/Archive/Add-
ons/Code_snippets/Preferences#Default_preferences
>
> 6) For d1efcd33ca6389479337f70a603c73c8264888b8:
> * Mozilla now uses Services.intl.DateTimeFormat(). We should too. See:
https://hg.mozilla.org/releases/mozilla-beta/rev/650bd331efd0
> * I don't think we should add a blank line after this one:
> let fracSecsStr = ...
> (that declaration is tied to the `for` loop that follows immediately
after).
Done.
I have pushed a new branch: bug25750_1
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/25750#comment:17>
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