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