Re: [tor-bugs] #23136 [Applications/Tor Launcher]: moat integration (fetch bridges for the user)

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#23136: moat integration (fetch bridges for the user)
--------------------------------------------+------------------------------
 Reporter:  mcs                             |          Owner:  brade
     Type:  defect                          |         Status:  needs_review
 Priority:  Very High                       |      Milestone:
Component:  Applications/Tor Launcher       |        Version:
 Severity:  Normal                          |     Resolution:
 Keywords:  TorBrowserTeam201802R, ux-team  |  Actual Points:
Parent ID:  #24689                          |         Points:
 Reviewer:                                  |        Sponsor:  Sponsor4
--------------------------------------------+------------------------------

Comment (by gk):

 That is an impressive work mcs and brade, especially given all the delays
 during this project and the workarounds you needed to find for those as
 well. Big Thanks!

 Here comes the first part of my review. It's mostly nits so far (5) has a
 question, though):

 1)

 oncommand="onBridgeTypeRadioChange()" is not properly aligned:

 {{{
 -                   label="&torsettings.useBridges.default;"
 selected="true"/>
 +                   label="&torsettings.useBridges.default;"
 selected="true"
 +                    oncommand="onBridgeTypeRadioChange()"/>
 }}}

 2)

 Copyright -> "2018" in network-settings-wizard.xul

 3)

 Copyright -> "2018" in network-settings.xul

 4) `let proxySettings;` as we are using it later on
 even if no proxy settings got specified should we initialize it somehow,
 say to `undefined`?

 5)

 if (!isUsingBridgeDBBridges)
 {
   gBridgeDBBridges = undefined;
   showBridgeDBBridges();
 }

 Why do we have the `showBridgeDBBridges()` call here if we are not using
 BridgeDB bridges (i.e. there
 aren't any to begin with)?

 6)

 Please, no mix of braces/non-braces style in if-else-clauses:
 {{{
 +      if (aErr.message)
 +        details = aErr.message;
 +      else if (aErr.code)
 +      {
 +        if (aErr.code < 1000)
 +          details = aErr.code;                     // HTTP status code
 +        else
 +          details = "0x" + aErr.code.toString(16); // nsresult
 +      }
 }}}
 {{{
         if (this._processStdoutLines())
           aResolve();
         else
         {
           // The PT handshake has not finished yet. Read more data.
           this._startStdoutRead(aResolve, aReject);
         }
 }}}

 7)

 It might not matter much but is socks4 really the thing we should test
 first
 (meaning the protocol we expect to get used in the majority of cases) when
 creating the proxy URL?

 8)

 "//  waitForCaptchaResponse" -> "// waitForCaptchaResponse" (and
 whitespace too much after the slashes)

 9)

 Do we need a spec update or an updated link to the spec you gave in `tl-
 bridgedb.jsm`? 'type': 'moat client supported transports' got changed
 'type':'client-transports' etc.
 So, https://github.com/isislovecruft/bridgedb/tree/fix/22871#requesting-
 bridges does
 not have latest version it seems?

 10)

 "Error Object" -> "Error object" and "a a text" in
 {{{
 // Extended Error Object which is used when we have a numeric code and a
 // a text error message.
 }}}

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