[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #30501 [Applications/Tor Browser]: BridgesList Preferences is an overloaded field
#30501: BridgesList Preferences is an overloaded field
-----------------------------------------------+---------------------------
Reporter: sisbell | Owner: tbb-team
Type: defect | Status:
| needs_review
Priority: Medium | Milestone:
Component: Applications/Tor Browser | Version:
Severity: Normal | Resolution:
Keywords: tbb-mobile, TorBrowserTeam201910R | Actual Points:
Parent ID: #31280 | Points: 2
Reviewer: | Sponsor:
| Sponsor30-can
-----------------------------------------------+---------------------------
Comment (by sysrqb):
`universal/src/main/java/com/msopentech/thali/toronionproxy/TorConfigBuilder.java`:
{{{
@@ -92,32 +92,19 @@ public final class TorConfigBuilder {
[snip]
if (!pluggableTransportClient.exists() ||
!pluggableTransportClient.canExecute()) {
throw new IOException("Bridge binary does not exist: " +
pluggableTransportClient
.getCanonicalPath());
}
}}}
Please make this exception's message reflect the correct error - does it
noe exist or is it not executable?
{{{
@@ -153,6 +140,27 @@ public final class TorConfigBuilder {
return
controlPortWriteToFile(context.config.getControlPortFile().getAbsolutePath());
}
+ public TorConfigBuilder customBridges(List<String> bridges) {
+ if(bridges.size() > 1) {
}}}
nit: "if ("
{{{
+ Collections.shuffle(bridges, new Random(System.nanoTime()));
+ }
}}}
I still don't completely understand this (or above). What's the reasoning
for shuffling? Tor chooses randomly from the list, in any case.
{{{
+ for (String bridge : bridges) {
+ if(!isNullOrEmpty(bridge)) {
+ line("Bridge " + bridge);
+ }
}}}
nit: Can you use `bridge()` instead of `line()` here? I think it'll be
more readable if `bridge()` is used consistently when we are adding a
bridge. (see below comment about `bridge()`, as well)
{{{
+ @SettingsConfig
+ public TorConfigBuilder customBridgesFromSettings() {
+ if (!settings.hasBridges() || !hasCustomBridges()) {
+ return this;
+ }
+ List<String> bridges = settings.getCustomBridges();
+ return customBridges(bridges);
}}}
Can you pass settings.getCustomBridges() directly into customBridges()?
{{{
+ /**
+ * Write up two bridges from packaged bridge list if bridges option
is enabled and if no custom bridges
}}}
nit: "Write up to"?
I also don't understand why it only writes two bridges? Why not write all
of them?
{{{
+ TorConfigBuilder addPredefinedBridgesFromResources(List<BridgeType>
types, int maxBridges) {
}}}
Should this have package-level visibility? I realize it was before this
change, too.
{{{
+ ArrayList<String> bridgeTypes = new ArrayList<>();
+ for(BridgeType bridgeType : types) {
}}}
nit: "for ("
{{{
+ bridgeTypes.add(bridgeType.name().toLowerCase());
+ }
+ if(settings.hasBridges() && hasPredefinedBridges() &&
!hasCustomBridges()) {
}}}
nit: "if ("
{{{
- if (b.type.equals(bridgeType)) {
+ if(bridgeTypes.contains(b.type)) {
}}}
"if ("
{{{
public TorConfigBuilder bridge(String type, String config) {
}}}
Can you change the name of this method to something like "addBridgeLine"?
"bridge()" is not descriptive.
{{{
+ private boolean hasPredefinedBridges() {
+ return !settings.getBridgeTypes().isEmpty();
}
}}}
Isn't this only testing that we have pre-defined bridge types, but not
actual bridges? We'd want to know if the stream for
`addPredefinedBridgesFromStream()` returns any bridges, right? Maybe
rename it as `hasPredefinedBridgeTypes()`? It seem like we also want to
know if we actually have predefined bridges in some cases, too. In
particular, `useBridgesFromSettings()` and
`addPredefinedBridgesFromResources()` should use this, right?
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/30501#comment:10>
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