Issue 4705284891082752: Proxy configurators

René Jeschke

Pretty huge review, sry. We need to take a generic look at the changes first ...

Aug. 11, 2014, 12:38 p.m. (2014-08-11 12:38:22 UTC) #1

Felix Dahlke

Did a first pass of reviewing this - but I'm pretty sure I'll need another ...

Aug. 19, 2014, 9:06 a.m. (2014-08-19 09:06:00 UTC) #2

René Jeschke

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/org/adblockplus/android/AdblockPlus.java File src/org/adblockplus/android/AdblockPlus.java (left): http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/org/adblockplus/android/AdblockPlus.java#oldcode231 src/org/adblockplus/android/AdblockPlus.java:231: // TODO: Why don't we re-check? On 2014/08/19 09:06:00, ...

Aug. 19, 2014, 10:41 a.m. (2014-08-19 10:41:33 UTC) #3

Felix Dahlke

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/org/adblockplus/android/BridgeCommand.java File src/org/adblockplus/android/BridgeCommand.java (right): http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/org/adblockplus/android/BridgeCommand.java#newcode48 src/org/adblockplus/android/BridgeCommand.java:48: final BridgeCommand e = MAP.get(str); On 2014/08/19 10:41:33, René ...

Aug. 19, 2014, 12:37 p.m. (2014-08-19 12:37:26 UTC) #4

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/...
File src/org/adblockplus/android/BridgeCommand.java (right):

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/...
src/org/adblockplus/android/BridgeCommand.java:48: final BridgeCommand e =
MAP.get(str);
On 2014/08/19 10:41:33, René Jeschke wrote:
> On 2014/08/19 09:06:00, Felix H. Dahlke wrote:
> > I guess I'm missing something here, but wouldn't BridgeCommand.valueOf(str)
> > basically do the trick?
> 
> 'valueOf' requires that 'str' exactly matches the enum name and throws an
> exception if the enum does not contain that name. I also don't know how the
> different enum values are queried (I don't think there's a hashmap behind).
> 
> Performance and readability wise this is the best (safest) solution.
> 

This seems overengineered. If we really do need a BridgeCommand.fromString that
never throws and that can handle case insensitivity, this would do the trick:

public static BridgeCommand fromString(final String str)
{
  try
  {
    return valueOf(str.toUpperCase(Locale.ENGLISH));
  }
  catch (Throwable t)
  {
    return UNKNOWN;
  }
}

That said, I do think the best place for this logic would be the caller, i.e.
ProxyService.

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/...
File src/org/adblockplus/android/ProxyService.java (right):

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/...
src/org/adblockplus/android/ProxyService.java:60: // FIXME remove me
On 2014/08/19 10:41:33, René Jeschke wrote:
> On 2014/08/19 09:06:00, Felix H. Dahlke wrote:
> > Why not remove it?
> 
> When I remove it, we don't have a way to enable request logging. That's why I
> currently keep this thing. I'll open an issue for that.

Can you remove the FIXME comment then? :)

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/...
src/org/adblockplus/android/ProxyService.java:98: // FIXME 'StrictMode' is API9
On 2014/08/19 10:41:33, René Jeschke wrote:
> On 2014/08/19 09:06:00, Felix H. Dahlke wrote:
> > Isn't our conclusion so far that API 9 should be our minimum supported
> version?
> > (https://issues.adblockplus.org/ticket/246)
> 
> Hm, there are devices out there that run API7 on a ARMv7 ... I thought we
talked
> about this ... we should talk about this again^^

Still - we're using API9 all over the place, don't we? If we do want to be API7
compatible, ADT should point this out, so no real point in the FIXME comment
IMO.

René Jeschke

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/org/adblockplus/android/BridgeCommand.java File src/org/adblockplus/android/BridgeCommand.java (right): http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/org/adblockplus/android/BridgeCommand.java#newcode48 src/org/adblockplus/android/BridgeCommand.java:48: final BridgeCommand e = MAP.get(str); On 2014/08/19 12:37:26, Felix ...

Aug. 19, 2014, 12:57 p.m. (2014-08-19 12:57:36 UTC) #5

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/...
File src/org/adblockplus/android/BridgeCommand.java (right):

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/...
src/org/adblockplus/android/BridgeCommand.java:48: final BridgeCommand e =
MAP.get(str);
On 2014/08/19 12:37:26, Felix H. Dahlke wrote:
> On 2014/08/19 10:41:33, René Jeschke wrote:
> > On 2014/08/19 09:06:00, Felix H. Dahlke wrote:
> > > I guess I'm missing something here, but wouldn't
BridgeCommand.valueOf(str)
> > > basically do the trick?
> > 
> > 'valueOf' requires that 'str' exactly matches the enum name and throws an
> > exception if the enum does not contain that name. I also don't know how the
> > different enum values are queried (I don't think there's a hashmap behind).
> > 
> > Performance and readability wise this is the best (safest) solution.
> > 
> 
> This seems overengineered. If we really do need a BridgeCommand.fromString
that
> never throws and that can handle case insensitivity, this would do the trick:
> 
> public static BridgeCommand fromString(final String str)
> {
>   try
>   {
>     return valueOf(str.toUpperCase(Locale.ENGLISH));
>   }
>   catch (Throwable t)
>   {
>     return UNKNOWN;
>   }
> }
> 
> That said, I do think the best place for this logic would be the caller, i.e.
> ProxyService.

I just don't know why you insist on replacing a fully automatic configured, O(1)
access without any memory allocations with:

1. Stack frame and monitor/sync setup for exception catching
2. Memory allocation for 'toUpperCase'
3. Possible O(n) access for iterating over the enum
4. And: it's 8 LOC compared to two now

And con 'logic at caller':
Not everything should be solved by throwing exceptions, exceptions are not flow
control, exceptions suck and are costly in all respects.
There is a 'UNKNOWN' value defined, which makes it a default/fallback value, so
why should I throw an exception?

But, if you really think that I should replace this, I will ... though it
definitely feels wrong^^

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/...
File src/org/adblockplus/android/ProxyService.java (right):

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/...
src/org/adblockplus/android/ProxyService.java:60: // FIXME remove me
On 2014/08/19 12:37:26, Felix H. Dahlke wrote:
> On 2014/08/19 10:41:33, René Jeschke wrote:
> > On 2014/08/19 09:06:00, Felix H. Dahlke wrote:
> > > Why not remove it?
> > 
> > When I remove it, we don't have a way to enable request logging. That's why
I
> > currently keep this thing. I'll open an issue for that.
> 
> Can you remove the FIXME comment then? :)

Done.

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/...
src/org/adblockplus/android/ProxyService.java:98: // FIXME 'StrictMode' is API9
On 2014/08/19 12:37:26, Felix H. Dahlke wrote:
> On 2014/08/19 10:41:33, René Jeschke wrote:
> > On 2014/08/19 09:06:00, Felix H. Dahlke wrote:
> > > Isn't our conclusion so far that API 9 should be our minimum supported
> > version?
> > > (https://issues.adblockplus.org/ticket/246)
> > 
> > Hm, there are devices out there that run API7 on a ARMv7 ... I thought we
> talked
> > about this ... we should talk about this again^^
> 
> Still - we're using API9 all over the place, don't we? If we do want to be
API7
> compatible, ADT should point this out, so no real point in the FIXME comment
> IMO.

Thing is, as you disliked my tries to remove the warnings from this project, 90%
is noise here. I definitely do find this FIXME again ... we need another pass to
remove warnings!
 ... still removed

Felix Dahlke

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/org/adblockplus/android/BridgeCommand.java File src/org/adblockplus/android/BridgeCommand.java (right): http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/org/adblockplus/android/BridgeCommand.java#newcode48 src/org/adblockplus/android/BridgeCommand.java:48: final BridgeCommand e = MAP.get(str); On 2014/08/19 12:57:36, René ...

Aug. 19, 2014, 1:14 p.m. (2014-08-19 13:14:57 UTC) #6

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/...
File src/org/adblockplus/android/BridgeCommand.java (right):

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/...
src/org/adblockplus/android/BridgeCommand.java:48: final BridgeCommand e =
MAP.get(str);
On 2014/08/19 12:57:36, René Jeschke wrote:
> On 2014/08/19 12:37:26, Felix H. Dahlke wrote:
> > On 2014/08/19 10:41:33, René Jeschke wrote:
> > > On 2014/08/19 09:06:00, Felix H. Dahlke wrote:
> > > > I guess I'm missing something here, but wouldn't
> BridgeCommand.valueOf(str)
> > > > basically do the trick?
> > > 
> > > 'valueOf' requires that 'str' exactly matches the enum name and throws an
> > > exception if the enum does not contain that name. I also don't know how
the
> > > different enum values are queried (I don't think there's a hashmap
behind).
> > > 
> > > Performance and readability wise this is the best (safest) solution.
> > > 
> > 
> > This seems overengineered. If we really do need a BridgeCommand.fromString
> that
> > never throws and that can handle case insensitivity, this would do the
trick:
> > 
> > public static BridgeCommand fromString(final String str)
> > {
> >   try
> >   {
> >     return valueOf(str.toUpperCase(Locale.ENGLISH));
> >   }
> >   catch (Throwable t)
> >   {
> >     return UNKNOWN;
> >   }
> > }
> > 
> > That said, I do think the best place for this logic would be the caller,
i.e.
> > ProxyService.
> 
> I just don't know why you insist on replacing a fully automatic configured,
O(1)
> access without any memory allocations with:
> 
> 1. Stack frame and monitor/sync setup for exception catching
> 2. Memory allocation for 'toUpperCase'
> 3. Possible O(n) access for iterating over the enum
> 4. And: it's 8 LOC compared to two now
> 
> And con 'logic at caller':
> Not everything should be solved by throwing exceptions, exceptions are not
flow
> control, exceptions suck and are costly in all respects.
> There is a 'UNKNOWN' value defined, which makes it a default/fallback value,
so
> why should I throw an exception?
> 
> But, if you really think that I should replace this, I will ... though it
> definitely feels wrong^^

I just get the very strong expression that this is trying to solve a problem we
don't have - not a good thing to complicate our code base with IMHO.

How I see it, this code attempts to solve two issues:

1. The caller will pass in strings that aren't going to match the case of the
enum value.
2. Getting enum values from strings is a performance bottleneck.
3. We need to fall back to UNKNOWN if there is no enum value for the supplied
string.

Here's my take on that:

1. This is not an actual problem, we have full control over the strings we pass
in. We can make all kinds of mistakes - I don't see why we should tackle case
sensitivity specifically.
2. I really doubt it's an actual bottleneck, the lookup should pale in
comparison to the broadcast overhead. If it is, we should definitely have a
comment describing this. (And I think it'd be more logical to have the
performance hack where the actual performance issue is, i.e. in the caller)
3. This can actually happen. I'd still prefer to handle it in the caller, mainly
because it seems unintuitive to have two ways of getting an enum value from a
string with different semantics. But I wouldn't insist on it.

If you don't agree, we should get a third opinion.

Felix Dahlke

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/org/adblockplus/android/ProxyService.java File src/org/adblockplus/android/ProxyService.java (right): http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/org/adblockplus/android/ProxyService.java#newcode98 src/org/adblockplus/android/ProxyService.java:98: // FIXME 'StrictMode' is API9 On 2014/08/19 12:57:36, René ...

Aug. 19, 2014, 1:17 p.m. (2014-08-19 13:17:50 UTC) #7

René Jeschke

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/org/adblockplus/android/BridgeCommand.java File src/org/adblockplus/android/BridgeCommand.java (right): http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/org/adblockplus/android/BridgeCommand.java#newcode48 src/org/adblockplus/android/BridgeCommand.java:48: final BridgeCommand e = MAP.get(str); On 2014/08/19 13:14:57, Felix ...

Aug. 19, 2014, 1:23 p.m. (2014-08-19 13:23:53 UTC) #8

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/...
File src/org/adblockplus/android/BridgeCommand.java (right):

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/...
src/org/adblockplus/android/BridgeCommand.java:48: final BridgeCommand e =
MAP.get(str);
On 2014/08/19 13:14:57, Felix H. Dahlke wrote:
> On 2014/08/19 12:57:36, René Jeschke wrote:
> > On 2014/08/19 12:37:26, Felix H. Dahlke wrote:
> > > On 2014/08/19 10:41:33, René Jeschke wrote:
> > > > On 2014/08/19 09:06:00, Felix H. Dahlke wrote:
> > > > > I guess I'm missing something here, but wouldn't
> > BridgeCommand.valueOf(str)
> > > > > basically do the trick?
> > > > 
> > > > 'valueOf' requires that 'str' exactly matches the enum name and throws
an
> > > > exception if the enum does not contain that name. I also don't know how
> the
> > > > different enum values are queried (I don't think there's a hashmap
> behind).
> > > > 
> > > > Performance and readability wise this is the best (safest) solution.
> > > > 
> > > 
> > > This seems overengineered. If we really do need a BridgeCommand.fromString
> > that
> > > never throws and that can handle case insensitivity, this would do the
> trick:
> > > 
> > > public static BridgeCommand fromString(final String str)
> > > {
> > >   try
> > >   {
> > >     return valueOf(str.toUpperCase(Locale.ENGLISH));
> > >   }
> > >   catch (Throwable t)
> > >   {
> > >     return UNKNOWN;
> > >   }
> > > }
> > > 
> > > That said, I do think the best place for this logic would be the caller,
> i.e.
> > > ProxyService.
> > 
> > I just don't know why you insist on replacing a fully automatic configured,
> O(1)
> > access without any memory allocations with:
> > 
> > 1. Stack frame and monitor/sync setup for exception catching
> > 2. Memory allocation for 'toUpperCase'
> > 3. Possible O(n) access for iterating over the enum
> > 4. And: it's 8 LOC compared to two now
> > 
> > And con 'logic at caller':
> > Not everything should be solved by throwing exceptions, exceptions are not
> flow
> > control, exceptions suck and are costly in all respects.
> > There is a 'UNKNOWN' value defined, which makes it a default/fallback value,
> so
> > why should I throw an exception?
> > 
> > But, if you really think that I should replace this, I will ... though it
> > definitely feels wrong^^
> 
> I just get the very strong expression that this is trying to solve a problem
we
> don't have - not a good thing to complicate our code base with IMHO.
> 
> How I see it, this code attempts to solve two issues:
> 
> 1. The caller will pass in strings that aren't going to match the case of the
> enum value.
> 2. Getting enum values from strings is a performance bottleneck.
> 3. We need to fall back to UNKNOWN if there is no enum value for the supplied
> string.
> 
> Here's my take on that:
> 
> 1. This is not an actual problem, we have full control over the strings we
pass
> in. We can make all kinds of mistakes - I don't see why we should tackle case
> sensitivity specifically.
> 2. I really doubt it's an actual bottleneck, the lookup should pale in
> comparison to the broadcast overhead. If it is, we should definitely have a
> comment describing this. (And I think it'd be more logical to have the
> performance hack where the actual performance issue is, i.e. in the caller)
> 3. This can actually happen. I'd still prefer to handle it in the caller,
mainly
> because it seems unintuitive to have two ways of getting an enum value from a
> string with different semantics. But I wouldn't insist on it.
> 
> If you don't agree, we should get a third opinion.

Well, k. But I won't leave exception handling for the caller.

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/...
File src/org/adblockplus/android/ProxyService.java (right):

http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/...
src/org/adblockplus/android/ProxyService.java:98: // FIXME 'StrictMode' is API9
On 2014/08/19 13:17:50, Felix H. Dahlke wrote:
> On 2014/08/19 12:57:36, René Jeschke wrote:
> > On 2014/08/19 12:37:26, Felix H. Dahlke wrote:
> > > On 2014/08/19 10:41:33, René Jeschke wrote:
> > > > On 2014/08/19 09:06:00, Felix H. Dahlke wrote:
> > > > > Isn't our conclusion so far that API 9 should be our minimum supported
> > > > version?
> > > > > (https://issues.adblockplus.org/ticket/246)
> > > > 
> > > > Hm, there are devices out there that run API7 on a ARMv7 ... I thought
we
> > > talked
> > > > about this ... we should talk about this again^^
> > > 
> > > Still - we're using API9 all over the place, don't we? If we do want to be
> > API7
> > > compatible, ADT should point this out, so no real point in the FIXME
comment
> > > IMO.
> > 
> > Thing is, as you disliked my tries to remove the warnings from this project,
> 90%
> > is noise here. I definitely do find this FIXME again ... we need another
pass
> to
> > remove warnings!
> >  ... still removed
> 
> Did I? I'm all for getting rid of warnings!

Yeah, think it's been unrelated changes back then, we'll need an issue for that
:-D

Felix Dahlke

Alright, will start the second pass then :) http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/org/adblockplus/android/ProxyService.java File src/org/adblockplus/android/ProxyService.java (right): http://codereview.adblockplus.org/4705284891082752/diff/5629499534213120/src/org/adblockplus/android/ProxyService.java#newcode98 src/org/adblockplus/android/ProxyService.java:98: // ...

Aug. 19, 2014, 1:28 p.m. (2014-08-19 13:28:51 UTC) #9

Felix Dahlke

Alright, second round done. Still looking good :) http://codereview.adblockplus.org/4705284891082752/diff/5088492568707072/src/org/adblockplus/android/ProxyServerType.java File src/org/adblockplus/android/ProxyServerType.java (right): http://codereview.adblockplus.org/4705284891082752/diff/5088492568707072/src/org/adblockplus/android/ProxyServerType.java#newcode27 src/org/adblockplus/android/ProxyServerType.java:27: /** ...

Aug. 19, 2014, 4:14 p.m. (2014-08-19 16:14:46 UTC) #10

René Jeschke

http://codereview.adblockplus.org/4705284891082752/diff/5088492568707072/src/org/adblockplus/android/ProxyServerType.java File src/org/adblockplus/android/ProxyServerType.java (right): http://codereview.adblockplus.org/4705284891082752/diff/5088492568707072/src/org/adblockplus/android/ProxyServerType.java#newcode27 src/org/adblockplus/android/ProxyServerType.java:27: /** SOCKS4 */ On 2014/08/19 16:14:46, Felix H. Dahlke ...

Aug. 19, 2014, 4:33 p.m. (2014-08-19 16:33:24 UTC) #11

http://codereview.adblockplus.org/4705284891082752/diff/5088492568707072/src/...
File src/org/adblockplus/android/ProxyServerType.java (right):

http://codereview.adblockplus.org/4705284891082752/diff/5088492568707072/src/...
src/org/adblockplus/android/ProxyServerType.java:27: /** SOCKS4 */
On 2014/08/19 16:14:46, Felix H. Dahlke wrote:
> I still don't see the point in including proxy methods we don't support here.
We
> have no way of registering anything other than a HTTP proxy right now and in
the
> foreseeable future, so that seems to suffice.

Removed.

http://codereview.adblockplus.org/4705284891082752/diff/5088492568707072/src/...
File src/org/adblockplus/android/ProxyService.java (right):

http://codereview.adblockplus.org/4705284891082752/diff/5088492568707072/src/...
src/org/adblockplus/android/ProxyService.java:97: // FIXME 'StrictMode' is API9
On 2014/08/19 16:14:46, Felix H. Dahlke wrote:
> I still think this should be part of a larger "support API 7 again" issue - if
> we do want that.

We talked about this some weeks ago. I told you there are devices with ARMv7 and
API7, you said: "then we need to support this", so, yes, I thought we would need
to go back to API7 :-D

But, jep, will create and issue :-D

FIXME removed.

http://codereview.adblockplus.org/4705284891082752/diff/5088492568707072/src/...
src/org/adblockplus/android/ProxyService.java:446: public boolean isManual()
On 2014/08/19 16:14:46, Felix H. Dahlke wrote:
> IMO it'd be neat to have isNative() and isIptables() in the same spirit, the
> former would be used more often than isManual() from what I can tell.
> 
> I _think_ we might be able to make getProxyRegistrationType() private then.

At least 'isIptables()'.

Done.

http://codereview.adblockplus.org/4705284891082752/diff/5088492568707072/src/...
src/org/adblockplus/android/ProxyService.java:473: case CYANOGENMOD:
On 2014/08/19 16:14:46, Felix H. Dahlke wrote:
> Do we currently filter non-wifi traffic on CyanogenMod? I thought not.
> 
> Actually, I think it would be cleaner if this logic would move into the
> individual configurators. enum ConnectionType { WIFI, DATA, ALL } or
something.

Of course we are filtering all connection traffic on CM, that's the point of it
all :-D (We're a global proxy there.)

Yes, could put this into a separate enum but we won't gain much as we still have
the special manual case.

http://codereview.adblockplus.org/4705284891082752/diff/5088492568707072/src/...
File src/org/adblockplus/android/Utils.java (right):

http://codereview.adblockplus.org/4705284891082752/diff/5088492568707072/src/...
src/org/adblockplus/android/Utils.java:47: LOCK.lock();
On 2014/08/19 16:14:46, Felix H. Dahlke wrote:
> Wouldn't a synchronize method have the same effect here?

Yeah, right, somehow I used a lock before, that method got copied from somewhere
else^^

Changed.

Felix Dahlke

http://codereview.adblockplus.org/4705284891082752/diff/5088492568707072/src/org/adblockplus/android/ProxyService.java File src/org/adblockplus/android/ProxyService.java (right): http://codereview.adblockplus.org/4705284891082752/diff/5088492568707072/src/org/adblockplus/android/ProxyService.java#newcode473 src/org/adblockplus/android/ProxyService.java:473: case CYANOGENMOD: On 2014/08/19 16:33:24, René Jeschke wrote: > ...

Aug. 19, 2014, 4:47 p.m. (2014-08-19 16:47:05 UTC) #12

Felix Dahlke

Looks good now, you didn't get back to me on this yet: http://codereview.adblockplus.org/4705284891082752/diff/5088492568707072/src/org/adblockplus/android/ProxyService.java#newcode473 I'll do ...

Aug. 23, 2014, 8:18 a.m. (2014-08-23 08:18:51 UTC) #13

René Jeschke

http://codereview.adblockplus.org/4705284891082752/diff/5088492568707072/src/org/adblockplus/android/ProxyService.java File src/org/adblockplus/android/ProxyService.java (right): http://codereview.adblockplus.org/4705284891082752/diff/5088492568707072/src/org/adblockplus/android/ProxyService.java#newcode473 src/org/adblockplus/android/ProxyService.java:473: case CYANOGENMOD: On 2014/08/19 16:47:05, Felix H. Dahlke wrote: ...

Aug. 23, 2014, 11:33 a.m. (2014-08-23 11:33:31 UTC) #14

Felix Dahlke

http://codereview.adblockplus.org/4705284891082752/diff/5655608640405504/src/org/adblockplus/android/CrashHandler.java File src/org/adblockplus/android/CrashHandler.java (right): http://codereview.adblockplus.org/4705284891082752/diff/5655608640405504/src/org/adblockplus/android/CrashHandler.java#newcode179 src/org/adblockplus/android/CrashHandler.java:179: final ProxyProperties proxyProperties = new ProxyProperties(this.host, p, this.excl); We ...

Aug. 24, 2014, 11:51 a.m. (2014-08-24 11:51:18 UTC) #15

René Jeschke

http://codereview.adblockplus.org/4705284891082752/diff/5655608640405504/src/org/adblockplus/android/CrashHandler.java File src/org/adblockplus/android/CrashHandler.java (right): http://codereview.adblockplus.org/4705284891082752/diff/5655608640405504/src/org/adblockplus/android/CrashHandler.java#newcode179 src/org/adblockplus/android/CrashHandler.java:179: final ProxyProperties proxyProperties = new ProxyProperties(this.host, p, this.excl); On ...

Aug. 24, 2014, 11:54 a.m. (2014-08-24 11:54:13 UTC) #16

LGTM!

Issue 4705284891082752: Proxy configurators (Closed)

Description

Patch Set 1 #

Patch Set 2 : Update 1 #

Patch Set 3 : FIXME removed #

Patch Set 4 : valueOf #

Patch Set 5 : Another round #

Patch Set 6 : Removed command bridge #

Patch Set 7 : Last batch of review issues #

Messages