[tor-bugs] #31286 [Applications/Tor Browser]: Include bridge configuration into about:preferences
Tor Bug Tracker & Wiki
blackhole at torproject.org
Wed Oct 2 18:46:23 UTC 2019
#31286: Include bridge configuration into about:preferences
-------------------------------------------------+-------------------------
Reporter: gk | Owner:
| pospeselr
Type: task | Status:
| needs_revision
Priority: High | Milestone:
Component: Applications/Tor Browser | Version:
Severity: Normal | Resolution:
Keywords: tbb-9.0-must-alpha, ff68-esr, ux- | Actual Points:
team, TorBrowserTeam201910 |
Parent ID: #10760 | Points: 15
Reviewer: | Sponsor:
| Sponsor44-can
-------------------------------------------------+-------------------------
Changes (by acat):
* keywords: tbb-9.0-must-alpha, ff68-esr, ux-team, TorBrowserTeam201910R
=> tbb-9.0-must-alpha, ff68-esr, ux-team, TorBrowserTeam201910
* status: needs_review => needs_revision
Comment:
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.
2. 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.
3. We should use `const` whenever possible, and `let` otherwise (#26184
+1).
4. 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).
5. I think strict (in)equality operators should (always?) be used (`!= ->
!==`, `==` -> `===`).
6. 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.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/31286#comment:29>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list