Include bridge configuration into about:preferences
Torbutton as a standalone extension is going away (#10760 (moved)) and while doing so we restructure our toolbar making it more usable by exposing New Identity directly on it (#27511 (moved)). However, we need to find a new home for the bridge configuration as well if we want to remove the onion button from the toolbar. The current plan is to move it to about:preferences as a general setting. This ticket tracks that work.
Activity
When you begin work on the coding side of this task, please start with the Tor Launcher code from the bug29197-01 branch within brade's tor-launcher repo (https://gitweb.torproject.org/user/brade/tor-launcher.git/log/?h=bug29197-01). Fixes which make Tor Launcher compatible with am ESR68-based Tor Browser are there (tickets #29197 (moved) and #31300 (moved)).
Background
As part of #29197 (moved), I'm working with this ticket. Also, I'm trying to cover some previous issues with this work, such as:
#24452 (moved) - Firewall option is visible behind Tor Network Settings... but not during start-up #27476 (moved) - Remove gap between Tor Launcher window and main browser window #25431 (moved) - "Tor is censored in my country" does not cover some scenarios
User flow
I'm working with this user flow in the same way we approached the network settings in TBA (#28329 (moved)). Even if we are not going to completely remove Tor Launcher when Tor Browser starts in this iteration, we can discuss this flow.
User Interface
For this iteration, I plan to keep almost the same user interface we currently have in Tor Launcher with the big aim of iterating over this during our sponsor30 work.
As we discussed before, we want to have a specific section in the regular Preferences section to configure Tor. That could be
about:preferences#network. It will look like:
== Network Settings
Tor Browser connects you to the Tor Network run by thousands of volunteers around the world. [Learn More]
Bridges
Bridges help you to access to the Tor network in places where Tor is blocked. This is dummy text now, but it should explain in plain words what is a bridge and how it can help them. [Learn More]
[ ] Use a bridge [ ] Select a bridge [ Choose... ] [ ] Request a bridge from torproject.org [ ] Provide a bridge I know
Advanced
Description? [Learn More]
[ ] Use a local proxy [ ] This computer goes through a firewall that only allows connections to specific ports
[ View logs ]
Notes
- Something I discovered during this process is that we (firefox) currently have Network Settings in
about:prefereces#general. Should we move it toabout:preferences#network? Should we remove proxy settings from Network Settings and keep it inabout:prefereces#general? - I really like Duncan's suggestions related to the bridge's flow. We should rework it during Sponsor30 and have it in consideration.
- If we are ok with this UI, the next step for me is creating mockups and coordinate with pospeselr for the implementation.
- Something I discovered during this process is that we (firefox) currently have Network Settings in
-
I agree with the idea of keeping the UI similar to what we currently have in Tor Launcher. We do not have a lot of time before this work needs to be finished, and we also do not have time to make changes to the startup wizard. Also, if we reuse most of the localized strings that will reduce localization effort. I also agree that in the longer run we should take a fresh look, i.e., during the sponsor30 project.
I am not sure where the settings should go in about:preferences. One approach would be to add a new
Tor Networkpanel (a peer of General/Home/Search/etc.).It would be great to de-emphasize the Firefox
Network Settingson the General panel, although some advanced users need to access it. Maybe add a warning message or alert? But we can tackle this after Tor Browser 9.0.One question about your proposed design: under
Advanced, will checking the[] Use a local proxybox cause the proxy settings to be displayed inline? I think that is OK, but Mozilla uses overlay panels when they want to push some settings deeper, e.g., for their Network Settings. 
Creating a new 'Tor Network' panel as a peer General, Home, Search, etc would probably be best in terms of UX, thought it is a bit more work than inserting the new stuff into General but that's fine.
I'm also personally in favor of nuking the existing 'Network Settings' in the General panel. For most users, it's just an easily accessible footgun. I think we can expect that users doing clever things with a custom Tor setup should be clever enough to edit
network.proxy.socksandnetwork.proxy.socks_portdirectly.mcs also brings up a good point about
Advancedthat I unfortunately think applies in general in the main views of about:preferences: the UI doesn't seem to change anywhere when you select option bubbles or select/deseelct checkboxes. Whereas the current tor-launcher Tor Network Settings page shows and hides UI elements based on options selected and boxes checked.I think we could reasonably display (without dynamically showing/hiding) all the input elements in the bridge selection UI, though maybe the 'I use a proxy...' and the 'This computer goes...' section should go into an overlay via an 'I have a weird computer network' button?
-
Thanks for the review folks!
-
Agreed about to keep the same strings so we can rely on our translation memory. Although, we could have a brief description for bridges and add the helper text at the learn more link which will go to the tor manual/support.tpo entry.
-
I think is better to have a different/specific section than General for Network Settings. Since we are Tor Browser,
Tor Network Settingsis a good name. Should we use the onion icon to refer to the Tor Network as well? or do we prefer a different icon? Currently, Firefox Icons don't have a Network one. -
If we keep Firefox's Network Settings in General Preferences, do we still need local proxy settings in
Tor Network Settings?
Based on your feedback, I made some mockups to visualize these settings:
[A] shows an initial state without inline settings.
[B] shows an initial state with inline settings disabled.
[C] shows Tor Network Settings with Bridges selected. Should we open MOAT in an overlay? Should we have inline settings for
Provide a bridge I know?
-
I'm also personally in favor of nuking the existing 'Network Settings' in the General panel. For most users, it's just an easily accessible footgun. I think we can expect that users doing clever things with a custom Tor setup should be clever enough to edit network.proxy.socks and network.proxy.socks_port directly. +1
If we keep Firefox's Network Settings in General Preferences, do we still need local proxy settings in Tor Network Settings? If I'm not mistaken, proxy in Firefox Network settings means the proxy that Firefox will use to connect to Internet (which should always be the local Tor client), while proxy in Tor Network Settings means the proxy that the Tor client will use to connect to the Internet.
So I think yes, if we were to keep Firefox Network settings, I think we still would need local proxy in Tor Network Settings (unless we want to repurpose the Firefox proxy settings UI, which I think might be confusing).
-
Hey antonela, I'll need a flat svg version of the Tor Browser icon please :)
Here's a stand-in with the correct dimensions I'm currently using: https://trac.torproject.org/projects/tor/attachment/ticket/31286/torNetworkIcon.svg
[A] looks good to me and acat is right here. FWIW I am fine with the general sentiment of hiding the Firefox Network settings while we are at it. Every option there should be configurable by pref and that should not be an overly hard burden for experienced users which is the only user group for the Firefox network settings in our context anyway.
-
progress pic for those watching at home: 
Working on wiring up the XUL with tor-launcher functionality now.
-
Good work pospeselr! Thanks for sharing the progress!
Let's talk about some different user flows we are going to have on this release, where the Tor Launcher and Tor network settings in
about:preferenceswill co-exist:1/
- open Tor Browser
- Tor launcher prompt, user press [connect]
- bootstrapping OK
- Tor Browser loads
about:tor
2/
- open Tor Browser
- Tor launcher prompt, user press [config], adds bridges
- Bootstrapping OK
- Tor Browser loads
about:tor
3/
- open Tor Browser
- Tor launcher prompt, user press [connect] and/or try [config]
- bootstrapping FAILS
- Tor Launcher warning message. User [config] bridges in Tor Launcher
4/
- open Tor Browser
- tor launcher prompt, user press [connect] and/or try [config]
- bootstrapping FAILS
- tor browser loads in about:preferences#network with a warning/notice message. Draft
- Tor Browser restarts
5/
- open Tor Browser
- tor launcher prompt, user press [connect] and/or try [config]
- bootstrapping FAILS
- tor browser loads in a warning screen. Draft
- Tor Browser restarts
I think that 3/ is the flow we should keep for this release. Am I right? What do you think folks? Do we all agree on this?
== To-do / Questions
-
Should remove Tor Network Settings item from the Tor button menu and rely on the [≡] menu > Preferences? If yes, I'll file that child ticket.
-
We need an onboarding card for explaining this change. Do we want to do it during this release? If yes, I'll file that child ticket.
-
We need a "view/copy Tor Log to clipboard" feature also in
about:preferences. pospeselr suggested having a popup in a XUL sub-dialog with user-selectable text and a 'copy to clipboard'. Do we need a child ticket for it as well?
== Future Work (S30)
In the future, we aim to have a user experience where Tor Browser prescinds of the Tor Launcher UI, and the launching experience is closer to regular browsers: the user clicks to the icon and the browser loads connected to Tor. If the bootstrapping fail, then Tor Browser shows a warning screen. This warning screen will give us room to run our diverse ideas for improving the Tor Browser on learning bridges, like MOAT, magic link, or anything new and fancy.
-
Bridge selection dialog, resizable.
-
We need an onboarding card for explaining this change. Do we want to do it during this release? If yes, I'll file that child ticket.
Done > https://trac.torproject.org/projects/tor/ticket/31768
-
Quick update on current progress over the last couple of weeks:
- golden path scenario for full BridgeDB CAPTCHA request, answer submission and bridge retrieval, displaying received bridges in the UI is working
- reading (and soon writing) of proxy settings from/to the Tor daemon is working (though UI still not hooked up)
- updated my dev branch to ESR68 (required a bit of XUL fixup)
latest ESR68-based dev branch (ESR60 changes squashed) : https://gitweb.torproject.org/user/richard/tor-browser.git/log/?h=bug_31286_v2 previous ESR60-based dev branch : https://gitweb.torproject.org/user/richard/tor-browser.git/log/?h=bug_31286
Unfortunately, the existing tor-launcher network/bridge settings logic is pretty tightly coupled with the UI logic, so I'm basically having to extract the core functionality and re-implement in
browser/components/torNetworkSettingsbut I suppose that's good long-term. -
 
-
Replying to pospeselr:
Hey antonela, I'll need a flat svg version of the Tor Browser icon please :)
Here's a stand-in with the correct dimensions I'm currently using: https://trac.torproject.org/projects/tor/attachment/ticket/31286/torNetworkIcon.svg
Attached
https://trac.torproject.org/projects/tor/attachment/ticket/31286/torNetworkIcon.2.svg
-
Ok, this work is about 98% of the way there so here's a review branch for your perusal:
tor-browser: https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_31286_review
This branch is 'feature complete' in the sense that there are probably bugs lurking and there is still a bit of polish to be done, but the bulk of the code is there. The remaining work is as follows:
- Pulling in strings from tor-button's dtd rather than hardcoding english
- Designing and implementing a way to get the tor daemon's logs
- Build a smarter way of sending SETCONF commands rather than hammering tor everytime the UI is interacted with
- Remove Firefox's network settings from the General preferences
- Update LearnMore links to point to real help URLs (currently pointing to example.com)
- Misc refactor/code cleanup/TODOs
Odds are if you see something squirrely there should be a TODO in the comments nearby but if not let me know and I'll add it to my list of work.
Almost the entire patch is new code, apart from includes in the existing preferences XUL and code. Hopefully it's organized well enough for easy review.
I did refactor SecurityLevel a bit, moving the strings out to a separate shared component used by both SecurityLevel and the TorNetworkSettings components (and other future integrations). Though in the future TorStrings can probably be pared down a lot as the reason it existed to begin with was because XUL falls over ungracefully if it references localization files that don't exist (as could happen with local Linux builds prior to the tor-button integration).
I will try and get Linux and macOS nightlies out for this ASAP so you all can have a look at it.
Trac:
Status: assigned to needs_review
Keywords: TorBrowserTeam201909 deleted, TorBrowserTeam201909R added What if you make Tor pane optional as
register_module("paneSync", gSyncPane);?+ // TODO: shouldn't this early out once meek settings are found?Probably, no. Current implementation follows the GCC option override logic.
// weird and depends on inferring our scenario basd on some firefox prefs and the*based
+ // relationship between the saved list of bridges in abotu:config vs the list saved in torrc*about
+ // builtin default types (obfs4, meek-azure, snowflake, etc) then we provide thecomma before "then" (from now on)
+ // "extensions.torlauncher.bridgedb_bridge."" branch. If they match *exactly* then we assumeextensions.torlauncher.bridgedb_bridge.*+ // about:config that tells us which scenario we are in so we don't have to guesscomma before "so" (from now on)
+ // TODO: put preffered bridge type first in thi slist*this list
+ // ie: same element in identical order*i.e. same
+ // clear bridge related firefox prefs*bridge-related Firefox
+// split on the first colon and any subsequent go into passwordrephrase
+ if (colonIndex < 0) { + // we don't log the contents of the potentially password containing string + throw new Error ("Invalid USERNAME:PASSWORD string");"No colon in the string" - that's all what was checked and can be thrown.
+// expects tring in the format: ADDRESS:PORT,ADDRESS:PORT,...*string
+// expects a '/n' delimited string of bridge string, which we split and trim*bridge strings
+ // TODO: saving to Tor would go in this callback when about:preferenes loses focus (ie user switches to another tab or closes)done?
+ Strings loaded from torbutton, but provide a fallback in case torbutton addon not enabledheh
+ bridgesDescription : getString(null, "Bridges help you to access the Tor network in places where Tor is blocked. This is dummy text now, but it should explain in plain words what is a bridge and how it can help them."),:)
-
Test builds of my bug_31286_review branch!
Linux: https://people.torproject.org/~richard/builds/tor-browser-linux64-tbb-nightly_en-US-31286.tar.xz macOS: https://people.torproject.org/~richard/builds/TorBrowser-tbb-nightly-osx64_en-US-31286.dmg
While I finish a first pass + testing: I think it would be good for new code to follow mozilla style rules. Especially since now it's quite easy to automatically fix these:
./mach lint -l eslint --fix browser/components/tornetworksettings browser/components/torstrings(fix in #31914 (moved) is needed).There are still a few remaining errors that have to be fixed manually, but I think this should not be very difficult. Many should be solved with
/* import-globals-from [...].js */, I think. Or trivial changes likeChromeUtils.import("re[/gre/modules/XPCOMUtils.jsm");](/gre/modules/XPCOMUtils.jsm");) -> const { XPCOMUtils } = ChromeUtils.import("re[/gre/modules/XPCOMUtils.jsm");.](/gre/modules/XPCOMUtils.jsm");`.) Or adding a missing import.Unless there are some concerns, I plan to review the result of running the above style-fixing command but without fixing manual errors (this is the result for me: https://github.com/acatarineu/tor-browser/commits/bug_31286_review_eslint).
Nice work!
I'm assuming we agree that we should follow mozilla coding style/practices for our JS code in tor-browser. If not, I think we should at least discuss it :) I haven't found a document that explains these in depth (other than https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_practices). But I think passing ESlint should be already a good start.
Other than that, I'm not sure how we should decide about JS coding conventions and practices that are not explicitly checked in ESLint. For example, there is #26184 (moved) which I agree with (we should use
constwhenever possible, andletotherwise).In any case, I'm suggesting some general (non-exhaustive) JS practices which I think are good. If we would agree, we could perhaps make these explicit somewhere?
- I personally think that using classes instead of the old "function + .prototype.*" idiom is preferable. For example, I find
class TorBridgeSettings { constructor() { this._bridgeSource = TorBridgeSource.NONE; this._selectedDefaultBridgeType = null; this._bridgeStrings = []; } get selectedDefaultBridgeType() { ... } static get defaultBridgeTypes() { ... } readDefaultBridges() { ... } ... }to be more readable than the
function TorBridgeSettingsversion. For me it is much easier to see what is a class (needs to be instantiated withnew) and what is just a 'regular' function, and also I find it less noisy, since you don't need to use all those.prototype = function() {over and over again. But converting these to class syntax should not be a blocker right now, I think.-
In the code, I see that some functions (not classes) and methods are capitalized and some are not. This also happens with code in
torbuttonandtor-launcher, but in general I think the convention with JS is that functions, methods and property names should be camel-case but not capitalized. That's also the case with JS code in Firefox, I think. -
We should use
constwhenever possible, andletotherwise (#26184 (moved) +1). -
I think
for (const x of y) {...}is preferrable toy.forEach(x => {...}(faster, and control flow likebreak,return,continue,await... work as one would expect). -
I think strict (in)equality operators should (always?) be used (
!= -> !==,==->===). -
We should use .jsm modules whenever possible, and only use "XUL" scripts (not sure how to call them, basically all the ones in '/content/*') when strictly needed. *.jsm scripts allow for explicit importing and exporting, and we do not have to worry about variables and functions going to the global scope, etc. Besides, *.jsm exports can be imported in other *.jsm but also in *.xul scripts, which is not true with exports in *.xul scripts.
I have not yet done a in-depth testing of the build, but I did a first pass on the code:
-
moz.build
I'm not sure torstrings should be a component. My understanding is that components have something to do with UI, and it's not the case of torstrings. More comments later on
torstrings/content/torStrings.js. -
preferences/in-content/preferences.js
Should be
/* import-globals-from ../../tornetworksettings/content/torNetworkPane.js */, that fixes agTorPane is undefinedeslint error. -
tornetworksettings/content/torBridgeDB.js
Why not
tornetworksettings/TorBridgeDB.jsm? Don't see it needs window or does anything with UI.In {{{ let svc = Cc["@torproject.org/torlauncher-protocol-service;1"].getService( Ci.nsISupports ); let gProtocolSvc = svc.wrappedJSObject; }}} Why not just: {{{ let gProtocolSvc = Cc["@torproject.org/torlauncher-protocol-service;1"].getService( Ci.nsISupports. wrappedJSObject ); }}}
-
tornetworksettings/content/torBridgeSettings.js:
Again, I think it should be a jsm module.
typos:
basd,abotu:configIt looks a bit strange that
TorBridgeSettings.DefaultBridgeTypes = function() {is a static function while the previous are getters. If the class syntax was used, you would be able to dostatic get DefaultBridgeTypes() {}and easily make it a getter as the others :){{{ // treat as an unordered set for shoving bridge types into let bridgeTypes = {}; }}} Better to use a
Setfor this I think.key.indexOf(aBridgeType) == 0->key.startsWith(aBridgeType)In
TorBridgeSettings.prototype._readTorrcBridges = function() {,retvalis declared outside an if, but only used inside. Besides, the function can return eitherfalse,Arrayorundefined, I think it would be better to explicitly return at the end. Actually, should this function always return an array? Otherwise inTorBridgeSettings.prototype.ReadSettings(and perhaps others) it will fail, since it's expecting an array (torrcBridges.length). BTW, thereply.lineArray.forEachblock can be written asreply.lineArray.map(x => x.trim()).filter(x => x), which is more compact, although I'm not completely sure which one is more readable.TorNoBridgeSettings,TorBuiltinBridgeSettings,TorBridgeDBBridgeSettings,TorUserProvidedBridgeSettings: I would suggest following the convention of only capitalizing class names. Besides, perhaps we could add a prefix likemakeTorNoBridgeSettingsorcreateTorNoBridgeSettings? I think it makes it clearer that these are factory functions. -
tornetworksettings/content/torFirewallSettings.js:
- jsm module?
- can we move
parseAddrPortListto some module and explicitly import it wherever it's needed? -
TorEmptyFirewallSettings,TorCustomFirewallSettings: same comment regarding factory functions as above.
-
tornetworksettings/content/torNetworkCategory.inc.xul:
- This is currently localized with Fluent, I guess we should decide what to do with this. I'm not sure we can use fluent yet (not supported by transifex), so if we use good old DTD entities the
data-l10nshould go away.
- This is currently localized with Fluent, I guess we should decide what to do with this. I'm not sure we can use fluent yet (not supported by transifex), so if we use good old DTD entities the
-
tornetworksettings/content/torNetworkPane.js:
- this one cannot be a JSM module, since it accesses
document:) - typos:
brige,about:preferenes - I would suggest moving
parsePort,parseAddrPort,parseUsernamePassword,parseAddrPortList,parseBridgeStrings,parsePortListto some module and importing explicitly wherever it's needed. - I would replace
.innerHTMLby.innerText, it's safer. - eslint complains that several functions are undefined (
TorBridgeSource,TorBridgeSettings,TorProxyType, ...) I would suggest importing them explicitly from the corresponding modules.
- this one cannot be a JSM module, since it accesses
-
tornetworksettings/content/torNetworkPane.xul:
- I think we should only need to load torNetworkPane.js here if we convert the others to jsm modules.
-
tornetworksettings/content/torProxySettings.js:
- jsm module :)
- typos:
addres - same previous comments about factory methods.
-
torstrings/content/torStrings.js:
- jsm module :)
- It's also used in securitysettings, so perhaps we could move it to
browser/modules/TorStrings.jsm?
Should we support building with
--without-tor-launcher? I mean disabling theTor Network SettingsUI if Tor Launcher is not available. I assume if one builds with that flag the UI inabout:preferenceswill be slightly broken.Trac:
Status: needs_review to needs_revision
Keywords: TorBrowserTeam201910R deleted, TorBrowserTeam201910 addedSuggestions for copy changes:
"Tor Browser routes your traffic over the Tor Network, run by thousands of volunteers around the world."
"Bridges help you access the Tor Network in places where Tor is blocked. Depending on where you are, one bridge may work better than another." (cut the following dummy text sentence)
"Provide a bridge" (cut "I know")
-
Replying to steph:
Suggestions for copy changes:
"Tor Browser routes your traffic over the Tor Network, run by thousands of volunteers around the world."
"Bridges help you access the Tor Network in places where Tor is blocked. Depending on where you are, one bridge may work better than another." (cut the following dummy text sentence)
"Provide a bridge" (cut "I know")
Thanks steph! Those suggestions are great. Pospeselr, let me know when those strings get published so we let emmapeel know about it. Thanks!
-
Ok, new tor-browser review branch with the remaining work completed and most if not all of acat's review feedback taken into account:
tor-browser: https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_31286_review2
I have a change to tor-launcher incoming which just includes new en-US strings that I'll try and get up tomorrow or this weekend, but wouldn't block an alpha build as en-US fallbacks also exist in the TorStrings module.
Trac:
Keywords: TorBrowserTeam201910 deleted, TorBrowserTeam201910R added
Status: needs_revision to needs_review Some more comments:
-
Do you think it's fine to fix automatic eslint issues via
./mach lint -l eslint --fix browser/modules/TorStrings.jsm browser/modules/TorProtocolService.jsm browser/modules/BridgeDB.jsm browser/components/torpreferences, or do you fear it might break something? In principle, it should only fix those which are safe. -
When opening
about:preferences, the 'Tor' text quickly disappears and then appears again. -
After setting "local proxy" (I tried localhost:9050) and restarting, the Tor section does not work again. See
browser/components/torpreferences/content/torProxySettings.jsmcomments, I think it's related to that. -
This is for UX probably, but in
Use local proxythere's no feedback about whether the introduced proxy was correctly configured or not (e.g. was there some error due to port being empty, or some typo, etc.) -
In browser console, there are some errors:
{{{ XML Parsing Error: undefined entity Location: moz-nullprincipal:{3424fa6d-a047-43e6-ad56-6c3b351213d8} Line Number 1, Column 143: }}}
perhaps this is expected because of missing strings, but not sure.
-
browser/components/preferences/in-content/main.js:
-
browser/components/preferences/in-content/main.xul:
- In other patches (e.g. #26345 (moved)) we went for hiding via css (
display:none). I'm not sure what is best though, so it's up to you (or if someone else has an opinion...).
- In other patches (e.g. #26345 (moved)) we went for hiding via css (
-
browser/components/torpreferences/content/torCategory.inc.xul
- These are localized via JS (not fluent), so perhaps we can remove the data-l10n attributes?
-
browser/components/torpreferences/content/parseFunctions.jsm:
- nit: in the error
Invalid PORT value, needs to be on range [0,65535] : '${port}'-> shouldn't say range [1,65535]?
- nit: in the error
-
browser/components/securitylevel/content/securityLevel.js
- // TODO: I'm pretty sure these lines are NO-OPs now as they return references to objects, rather
// than populating the global scope?
- Unfortunately, I think
ChromeUtils.importdoes have side-effects, meaning that the exported symbols are populated as "globals" in the scope it's executed. If that's a *.jsm, then those will be available only in that *.jsm. If it's a XUL script, then it will be available for other XUL scripts too. But that's ok here, I think.
- Unfortunately, I think
- // TODO: I'm pretty sure these lines are NO-OPs now as they return references to objects, rather
// than populating the global scope?
-
browser/components/torpreferences/content/requestBridgeDialog.jsm
- The
TorProtocolServiceimport is unused (eslint error).
- The
-
browser/components/torpreferences/content/requestBridgeDialog.jsm:
- typo:
use strinct; -
// user may have opened a Request Bridge dialog in another tab, so update the-> It seems the end of the comment got cut. - Not a blocker, but FWIW: Almost all usage of
selfis not needed, since it's done inside arrow functions, which capture the value ofthiswhen they are created, so in all those cases self === this. It might be needed in the only place where a regular function is used: {{{ window.setTimeout(function() { self._populateXUL(dialog); }, 0); }}} Which might be converted into an arrow function and thenselfwould not be needed for sure (This also valid in other files). - this._captchaHbox is unused.
- Separe
_captchaGuessIsEmptyandinitmethods with a blank line? - nit: comment
// disable submit... if input value is empty? - When you click "reload/get another captcha", shouldn't the "solution not correct" text be hidden when you get the new captcha? It's not happening for me.
- typo:
-
browser/components/torpreferences/content/torBridgeSettings.jsm:
- A couple of try/catch for
Services.prefs.getCharPrefcan be removed by providing a default value (e.g.let defaultBridgeType = Services.prefs.getCharPref(TorStrings.preferenceKeys.defaultBridgeType, null);. ESlint is complaining about those.
- A couple of try/catch for
-
browser/components/torpreferences/content/torFirewallSettings.jsm:
-
parseAddrPortListis undefined, I think it should be imported.
-
-
browser/components/torpreferences/content/torPane.js
- eslint complains about
gSubDialognot being defined, but I see this also happening onprivacy.jsorhome.jswhich we did not touch so I guess we can ignore this.
- eslint complains about
-
browser/modules/TorStrings.jsm:
- I think at some point we should avoid the hardcoded english string fallbacks, perhaps using the fact that we have en-US torbutton locales always available and use those. But perhaps we can use a different ticket for that later.
-
browser/components/torpreferences/content/torProxySettings.jsm:
- several parse* functions are undefined, I think they should be imported (actually I thought these would not really throw errors but at least for me they do, on a restart in readSettings).
- in
readSettings,tlpsandreplyare never used.
Trac:
Keywords: TorBrowserTeam201910R deleted, TorBrowserTeam201910 added
Status: needs_review to needs_revision-
-
tor-browser review branch: https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_31286_review3
tor-browser dev branch starting at review2 : https://gitweb.torproject.org/user/richard/tor-browser.git/log/?h=bug_31286_v3
tor-launcher review branch: https://gitweb.torproject.org/user/richard/tor-launcher.git/commit/?h=bug_31286_review
acat: I'm inclined to agree with you surrounding the error handling around inputting proxy (and other) settings. I think we need a subsequent ticket surrounding UX improvements around what good error messaging/prompting looks like in the about:preferences context.
I still need to do some string shuffling/editing in tor-launcher which I'm doing next.
EDIT: We also need a page to point to for the 'Learn More' link under 'Advanced' (proxy, firewall settings). Or we can just remove that 'Learn More' link.
Trac:
Status: needs_revision to needs_review
Keywords: TorBrowserTeam201910 deleted, TorBrowserTeam201910R added -
Awesome work! The changes in
bug_31286_review3look reasonable to me. They still need a closer look, but I think we can use what we have for the alpha release.I merged
bug_31286_review3to `tor-browser-68.1.0esr-9.0-2´ (commit bd4082f2f1db8a1f1c135d6d1689242f7c659a19).Some nits:
- You now have a
"strict;";in yourBridgeDB.jsm. (I fixed this in a fixup commit (109c1defa853ec6364c66a72b3554ea05304dd3f)) - You now have
// console.log(`${setting} : ${value}`);in
TorProtocolService.jsm. If we don't want to log don't comment the code just delete it. - You now have a
-
Trac:
31286_stringfix.sh -
I've since amended the tor-launcher review branch to include the required fixes (removing colons, migrating some existing strings from torlauncher.properties using the attached script: https://trac.torproject.org/projects/tor/attachment/ticket/31286/31286_stringfix.sh )
