[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_revision
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 sisbell):
Replying to [comment:10 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?
Sure, easy enough
>
> {{{
> @@ -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 ("
>
+1
> {{{
> + 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.
I wasn't aware that tor itself does the shuffling. I will remove this
line.
>
> {{{
> + 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)
line method is a general method for just writing out a line. Its also used
in torrcCustomFromSettings method. I can change this is writeLine to make
clearer.
>
> {{{
> + @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()?
Sure, its a stylistic preference but easy enough.
>
>
> {{{
> + /**
> + * 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?
I thought this was a requirement but if not, I'll remove it.
>
> {{{
> + TorConfigBuilder addPredefinedBridgesFromResources(List<BridgeType>
types, int maxBridges) {
> }}}
> Should this have package-level visibility? I realize it was before this
change, too.
I'll change it. I left some open for the unit tests but this method is not
under direct test.
>
> {{{
> + 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.
The TorConfigBuilder just maps method name to fieldName. If I changed this
one I would need to do such for all methods, which I think would junk up
the readability of all the public methods, as they would all be
add{FieldName}Line.
>
> {{{
> + 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()`?
Makes sense, I'll rename method to be clearer
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?
This would occur if the bridges.txt is empty. This check occurs in
readBridgesFromStream, which would return an empty list.
addPredefinedBridgesFromStream invokes readBridgesFromStream so that is
where the check is currently occurring (an empty list is effectively a
noop). I can document this to be clearer.
I'll need to investigate further the case of useBridgesFromSettings if the
list is empty, in that case it may require explicit check.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/30501#comment:12>
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