Opened 4 months ago

Closed 4 weeks ago

#31286 closed task (fixed)

Include bridge configuration into about:preferences

Reported by: gk Owned by: pospeselr
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-9.0-must-alpha, ff68-esr, ux-team, TorBrowserTeam201910R
Cc: pospeselr, tbb-team, intrigeri Actual Points: 45
Parent ID: Points: 15
Reviewer: Sponsor: Sponsor44-can

Description

Torbutton as a standalone extension is going away (#10760) and while doing so we restructure our toolbar making it more usable by exposing New Identity directly on it (#27511). 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.

Child Tickets

Attachments (12)

A-about:preferences#network.png (237.2 KB) - added by antonela 3 months ago.
B-about:preferences#network.png (277.8 KB) - added by antonela 3 months ago.
C-about:preferences#network.png (190.6 KB) - added by antonela 3 months ago.
torNetworkIcon.svg (159 bytes) - added by pospeselr 3 months ago.
temp stand-in icon
progress.png (167.1 KB) - added by pospeselr 3 months ago.
2-warning-fullpage.png (270.0 KB) - added by antonela 3 months ago.
1-warning-about:preferences#network.png (304.1 KB) - added by antonela 3 months ago.
request-bridge.png (118.5 KB) - added by pospeselr 3 months ago.
progress00.png (178.8 KB) - added by pospeselr 8 weeks ago.
progress01.png (153.0 KB) - added by pospeselr 8 weeks ago.
torNetworkIcon.2.svg (1.3 KB) - added by antonela 8 weeks ago.
31286_stringfix.sh (1.4 KB) - added by pospeselr 5 weeks ago.

Download all attachments as: .zip

Change History (58)

comment:1 Changed 3 months ago by mcs

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 and #31300).

comment:2 Changed 3 months ago by pili

Sponsor: Sponsor44-can

Adding Sponsor 44 to ESR68 tickets

comment:3 Changed 3 months ago by pospeselr

Owner: changed from tbb-team to pospeselr
Status: newassigned

comment:4 Changed 3 months ago by pospeselr

Cc: tbb-team added

comment:5 Changed 3 months ago by antonela

Background

As part of #29197, I'm working with this ticket. Also, I'm trying to cover some previous issues with this work, such as:

#24452 - Firewall option is visible behind Tor Network Settings... but not during start-up
#27476 - Remove gap between Tor Launcher window and main browser window
#25431 - "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). Even if we are not going to completely remove Tor Launcher when Tor Browser starts in this iteration, we can discuss this flow.

https://trac.torproject.org/projects/tor/raw-attachment/ticket/28329/userflow-1.1.png

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 to about:preferences#network? Should we remove proxy settings from Network Settings and keep it in about: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.

comment:6 Changed 3 months ago by mcs

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 Network panel (a peer of General/Home/Search/etc.).

It would be great to de-emphasize the Firefox Network Settings on 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 proxy box 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.

comment:7 Changed 3 months ago by pospeselr

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.socks and network.proxy.socks_port directly.

mcs also brings up a good point about Advanced that 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?

Changed 3 months ago by antonela

Changed 3 months ago by antonela

Changed 3 months ago by antonela

comment:8 Changed 3 months ago by antonela

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 Settings is 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.

https://trac.torproject.org/projects/tor/raw-attachment/ticket/31286/A-about%3Apreferences%23network.png

[B] shows an initial state with inline settings disabled.

https://trac.torproject.org/projects/tor/raw-attachment/ticket/31286/B-about%3Apreferences%23network.png

[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?

https://trac.torproject.org/projects/tor/raw-attachment/ticket/31286/C-about%3Apreferences%23network.png

comment:9 Changed 3 months ago by acat

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).

comment:10 Changed 3 months ago by 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

Last edited 3 months ago by pospeselr (previous) (diff)

Changed 3 months ago by pospeselr

Attachment: torNetworkIcon.svg added

temp stand-in icon

comment:11 Changed 3 months ago by gk

[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.

comment:12 Changed 3 months ago by pospeselr

progress pic for those watching at home: https://trac.torproject.org/projects/tor/raw-attachment/ticket/31286/progress.png

Working on wiring up the XUL with tor-launcher functionality now.

Last edited 3 months ago by antonela (previous) (diff)

Changed 3 months ago by pospeselr

Attachment: progress.png added

comment:13 Changed 3 months ago by antonela

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:preferences will 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.

Last edited 3 months ago by antonela (previous) (diff)

Changed 3 months ago by antonela

Attachment: 2-warning-fullpage.png added

Changed 3 months ago by antonela

Changed 3 months ago by pospeselr

Attachment: request-bridge.png added

comment:14 Changed 3 months ago by pospeselr

Bridge selection dialog, resizable.

https://trac.torproject.org/projects/tor/raw-attachment/ticket/31286/request-bridge.png

Last edited 3 months ago by pospeselr (previous) (diff)

comment:15 Changed 2 months ago by intrigeri

Cc: intrigeri added

comment:16 Changed 2 months ago by gk

Keywords: tbb-9.0-must-alpha TorBrowserTeam201909 added; TorBrowserTeam201908 removed
Priority: MediumHigh

comment:17 Changed 2 months ago by pili

Points: 15

comment:18 Changed 2 months ago by antonela

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

comment:19 Changed 8 weeks ago by pospeselr

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/torNetworkSettings but I suppose that's good long-term.

Changed 8 weeks ago by pospeselr

Attachment: progress00.png added

Changed 8 weeks ago by pospeselr

Attachment: progress01.png added

comment:20 Changed 8 weeks ago by pospeselr

https://trac.torproject.org/projects/tor/raw-attachment/ticket/31286/progress00.png
https://trac.torproject.org/projects/tor/raw-attachment/ticket/31286/progress01.png

Changed 8 weeks ago by antonela

Attachment: torNetworkIcon.2.svg added

comment:21 in reply to:  10 Changed 8 weeks ago by antonela

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

comment:22 Changed 8 weeks ago by pospeselr

Thanks antonela :)

comment:23 Changed 6 weeks ago by pospeselr

Keywords: TorBrowserTeam201909R added; TorBrowserTeam201909 removed
Status: assignedneeds_review

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.

comment:24 Changed 6 weeks ago by cypherpunks

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 the

comma before "then" (from now on)

+  // "extensions.torlauncher.bridgedb_bridge."" branch. If they match *exactly* then we assume

extensions.torlauncher.bridgedb_bridge.*

+  // about:config that tells us which scenario we are in so we don't have to guess

comma 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 password

rephrase

+  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 enabled

heh

+    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."),

:)

comment:25 Changed 6 weeks ago by antonela

Cc: antonela removed
Keywords: ux-team added

comment:27 Changed 6 weeks ago by pili

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201909R removed

We're now in October, moving September outstanding reviews to October

comment:28 Changed 6 weeks ago by acat

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 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 like ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm"); -> const { XPCOMUtils } = ChromeUtils.import("resource://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).

comment:29 Changed 6 weeks ago by acat

Keywords: TorBrowserTeam201910 added; TorBrowserTeam201910R removed
Status: needs_reviewneeds_revision

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 which I agree with (we should use const whenever possible, and let otherwise).

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?

  1. 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 TorBridgeSettings version. For me it is much easier to see what is a class (needs to be instantiated with new) 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.

  1. In the code, I see that some functions (not classes) and methods are capitalized and some are not. This also happens with code in torbutton and tor-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.
  1. We should use const whenever possible, and let otherwise (#26184 +1).
  1. I think for (const x of y) {...} is preferrable to y.forEach(x => {...} (faster, and control flow like break, return, continue, await... work as one would expect).
  1. I think strict (in)equality operators should (always?) be used (!= -> !==, == -> ===).
  1. 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 a gTorPane is undefined eslint 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:config

It 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 do static 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 Set for this I think.

key.indexOf(aBridgeType) == 0 -> key.startsWith(aBridgeType)

In TorBridgeSettings.prototype._readTorrcBridges = function() {, retval is declared outside an if, but only used inside. Besides, the function can return either false, Array or undefined, I think it would be better to explicitly return at the end. Actually, should this function always return an array? Otherwise in TorBridgeSettings.prototype.ReadSettings (and perhaps others) it will fail, since it's expecting an array (torrcBridges.length). BTW, the reply.lineArray.forEach block can be written as reply.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 like makeTorNoBridgeSettings or createTorNoBridgeSettings? I think it makes it clearer that these are factory functions.

  • tornetworksettings/content/torFirewallSettings.js:
    • jsm module?
    • can we move parseAddrPortList to 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-l10n should go away.
  • 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, parsePortList to some module and importing explicitly wherever it's needed.
    • I would replace .innerHTML by .innerText, it's safer.
    • eslint complains that several functions are undefined (TorBridgeSource, TorBridgeSettings, TorProxyType, ...) I would suggest importing them explicitly from the corresponding modules.
  • 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 the Tor Network Settings UI if Tor Launcher is not available. I assume if one builds with that flag the UI in about:preferences will be slightly broken.

comment:30 Changed 5 weeks ago by 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")

comment:31 in reply to:  30 Changed 5 weeks ago by antonela

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!

comment:32 Changed 5 weeks ago by pospeselr

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed
Status: needs_revisionneeds_review

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.

comment:33 Changed 5 weeks ago by acat

Keywords: TorBrowserTeam201910 added; TorBrowserTeam201910R removed
Status: needs_reviewneeds_revision

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.jsm comments, I think it's related to that.
  • This is for UX probably, but in Use local proxy there'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) 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...).
  • 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]?
  • 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.import does 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.
  • browser/components/torpreferences/content/requestBridgeDialog.jsm
    • The TorProtocolService import is unused (eslint error).
  • 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 self is not needed, since it's done inside arrow functions, which capture the value of this when 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 then self would not be needed for sure (This also valid in other files).
    • this._captchaHbox is unused.
    • Separe _captchaGuessIsEmpty and init methods 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.
  • browser/components/torpreferences/content/torBridgeSettings.jsm:
    • A couple of try/catch for Services.prefs.getCharPref can be removed by providing a default value (e.g. `let defaultBridgeType = Services.prefs.getCharPref(TorStrings.preferenceKeys.defaultBridgeType, null);`. ESlint is complaining about those.
  • browser/components/torpreferences/content/torFirewallSettings.jsm:
    • parseAddrPortList is undefined, I think it should be imported.
  • browser/components/torpreferences/content/torPane.js
    • eslint complains about gSubDialog not being defined, but I see this also happening on privacy.js or home.js which we did not touch so I guess we can ignore this.
  • 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, tlps and reply are never used.

comment:34 Changed 5 weeks ago by pospeselr

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed
Status: needs_revisionneeds_review

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.

Last edited 5 weeks ago by pospeselr (previous) (diff)

comment:35 Changed 5 weeks ago by gk

Awesome work! The changes in bug_31286_review3 look 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_review3 to `tor-browser-68.1.0esr-9.0-2´ (commit bd4082f2f1db8a1f1c135d6d1689242f7c659a19).

Some nits:

1) You now have a "strict;"; in your BridgeDB.jsm. (I fixed this in a fixup commit (109c1defa853ec6364c66a72b3554ea05304dd3f))
2) 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.

Changed 5 weeks ago by pospeselr

Attachment: 31286_stringfix.sh added

comment:36 Changed 5 weeks ago by pospeselr

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 )

comment:37 in reply to:  36 Changed 4 weeks ago by gk

Replying to pospeselr:

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 )

Could you base your patch on tor-launcher's master and provide the changes on top of the ones you made and that got already committed?

comment:38 in reply to:  35 Changed 4 weeks ago by acat

Replying to gk:

Awesome work! The changes in bug_31286_review3 look 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_review3 to `tor-browser-68.1.0esr-9.0-2´ (commit bd4082f2f1db8a1f1c135d6d1689242f7c659a19).

Some nits:

1) You now have a "strict;"; in your BridgeDB.jsm. (I fixed this in a fixup commit (109c1defa853ec6364c66a72b3554ea05304dd3f))
2) 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.

Changes in bug_31286_review3 also LGTM. I might have missed some things in the review (e.g. console.log that gk pointed out), but the ones I mentioned were addressed there.

comment:39 Changed 4 weeks ago by gk

Keywords: TorBrowserTeam201910 added; TorBrowserTeam201910R removed
Status: needs_reviewneeds_revision

comment:40 Changed 4 weeks ago by pospeselr

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed
Status: needs_revisionneeds_review

comment:41 in reply to:  40 ; Changed 4 weeks ago by gk

Keywords: TorBrowserTeam201910 added; TorBrowserTeam201910R removed
Status: needs_reviewneeds_revision

Replying to pospeselr:

tor-launcher (rebased on latest): https://gitweb.torproject.org/user/richard/tor-launcher.git/commit/?h=bug_31286_review2&id=5f40953122f759fe6a4992f7a61e4cb3d5888d53

Thanks, I picked that up in commit 017ca3ee046af4b1afa641f772834d373ff750c7.

However, it seems we essentially have duplicated strings now, right? One version in the .properties file and one in the .dtd one. Any reason you did not use the technique Alex used in #24653 to avoid adding duplicated strings?

tor-browser fixup: https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_31286_review4

I am fine with the decision to omit the link for now, however, could you make that a) a fixup commit to your previous one and b) open a new ticket to get the correct fix done (that is (i) having actually a link working and (b) removing your hiding the link)?

comment:42 in reply to:  41 ; Changed 4 weeks ago by pospeselr

I've amended the review4 branch to change the last commit to a fixup! commit:

tor-browser: tor-browser fixup: ​https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_31286_review4

gk: I opted to move the existing strings to the DTD as I gathered from acat's changes to securityLevel.js that DTD was what we were using going forward (since those strings had originally been in a properties file in tor-button).

Once tor-launcher is gone and its functionality has been completely integrated into Tor Browser (9.5?) then the tor-launcher (and maybe also tor-button?) strings can live nicely together in one place in the tor-browser repo and the 'TorStrings' module can mostly go away. I think it would make sense to do the string merge at that point.

Given that new tor bootstrapping UX is going to be coming down the pipe soonish, do we need --without-tor-launcher support? What dev scenarios does that enable? If so, I'll create a bug to track doing that.

EDIT: Created #32120 to track the hidden 'Learn More' link

Last edited 4 weeks ago by pospeselr (previous) (diff)

comment:43 Changed 4 weeks ago by pospeselr

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed
Status: needs_revisionneeds_review

comment:44 in reply to:  42 Changed 4 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

Replying to pospeselr:

I've amended the review4 branch to change the last commit to a fixup! commit:

tor-browser: tor-browser fixup: ​https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_31286_review4

Thanks! git commit --fixup <commitID to fix up> is what you want next time. ;) Othewise git rebase has trouble to figure out which commit you actually want to fix up. I did that in commit 569fa462103fdd3547da0a44cffae51b2011b2b6 on tor-browser-68.1.0esr-9.0-3.

gk: I opted to move the existing strings to the DTD as I gathered from acat's changes to securityLevel.js that DTD was what we were using going forward (since those strings had originally been in a properties file in tor-button).

Once tor-launcher is gone and its functionality has been completely integrated into Tor Browser (9.5?) then the tor-launcher (and maybe also tor-button?) strings can live nicely together in one place in the tor-browser repo and the 'TorStrings' module can mostly go away. I think it would make sense to do the string merge at that point.

Sounds good.

Given that new tor bootstrapping UX is going to be coming down the pipe soonish, do we need --without-tor-launcher support? What dev scenarios does that enable? If so, I'll create a bug to track doing that.

We can think about that once the plans are a bit more mature. Right now the configure switch is useful as we don't build Tor Launcher for mobile e.g. as it is not needed there.

EDIT: Created #32120 to track the hidden 'Learn More' link

comment:45 Changed 4 weeks ago by pospeselr

Actual Points: 45
Parent ID: #10760
Resolution: fixed
Status: closedreopened

Should note the original points estimate was remaining work from the start of September, while the total points takes into account my work starting July 31. The Actual Points starting from September 2nd is '27'.

comment:46 Changed 4 weeks ago by pospeselr

Resolution: fixed
Status: reopenedclosed
Note: See TracTickets for help on using tickets.