[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #31286 [Applications/Tor Browser]: Include bridge configuration into about:preferences



#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:

 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.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/31286#comment:33>
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