#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@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs