Issue 1771 - Update RootTools

Issue 1771 - Update RootTools

[Felix Dahlke, 2014-11-19 10:38:39]

Just a couple of nits.

http://codereview.adblockplus.org/5624117646589952/diff/5685265389584384/src/...
File src/org/adblockplus/android/configurators/IptablesProxyConfigurator.java
(right):

http://codereview.adblockplus.org/5624117646589952/diff/5685265389584384/src/...
src/org/adblockplus/android/configurators/IptablesProxyConfigurator.java:58:
private static List<String> sendShell(final String command, final int timeout)
throws IOException, TimeoutException,
I think "sendShell" is not a great name, I can see why they changed it. How
about going with "runRootCommand" or something similar here instead, for
clarity?

http://codereview.adblockplus.org/5624117646589952/diff/5685265389584384/src/...
src/org/adblockplus/android/configurators/IptablesProxyConfigurator.java:259:
private final static class CommandOutput extends Command
Shouldn't this rather be "OutputCommand" (or anything else ending in "Command"),
since it's a command, not an "output"?

http://codereview.adblockplus.org/5624117646589952/diff/5685265389584384/src/...
src/org/adblockplus/android/configurators/IptablesProxyConfigurator.java:261:
private final Semaphore completionNotify = new Semaphore(1);
This confused me a little bit, being a verb (bit unusual for field names). How
about just "running" or something?

[René Jeschke, 2014-11-24 13:07:57]

http://codereview.adblockplus.org/5624117646589952/diff/5685265389584384/src/...
File src/org/adblockplus/android/configurators/IptablesProxyConfigurator.java
(right):

http://codereview.adblockplus.org/5624117646589952/diff/5685265389584384/src/...
src/org/adblockplus/android/configurators/IptablesProxyConfigurator.java:58:
private static List<String> sendShell(final String command, final int timeout)
throws IOException, TimeoutException,
On 2014/11/19 10:38:39, Felix H. Dahlke wrote:
> I think "sendShell" is not a great name, I can see why they changed it. How
> about going with "runRootCommand" or something similar here instead, for
> clarity?

Right, makes sense, will change this.

http://codereview.adblockplus.org/5624117646589952/diff/5685265389584384/src/...
src/org/adblockplus/android/configurators/IptablesProxyConfigurator.java:259:
private final static class CommandOutput extends Command
On 2014/11/19 10:38:39, Felix H. Dahlke wrote:
> Shouldn't this rather be "OutputCommand" (or anything else ending in
"Command"),
> since it's a command, not an "output"?

Right, will also change this.

http://codereview.adblockplus.org/5624117646589952/diff/5685265389584384/src/...
src/org/adblockplus/android/configurators/IptablesProxyConfigurator.java:261:
private final Semaphore completionNotify = new Semaphore(1);
On 2014/11/19 10:38:39, Felix H. Dahlke wrote:
> This confused me a little bit, being a verb (bit unusual for field names). How
> about just "running" or something?

"running" as name for a flag would mean sense, but I'm using a Semaphore here,
which is used for waiting/counting.

I will come up with something different, still.

[Felix Dahlke, 2015-01-09 18:34:08]

This doesn't really seem to solve #1498, so I've created a dedicated issue for
this update: https://issues.adblockplus.org/ticket/1771

Please change the title :)

[René Jeschke, 2015-01-23 13:37:36]

http://codereview.adblockplus.org/5624117646589952/diff/5685265389584384/src/...
File src/org/adblockplus/android/configurators/IptablesProxyConfigurator.java
(right):

http://codereview.adblockplus.org/5624117646589952/diff/5685265389584384/src/...
src/org/adblockplus/android/configurators/IptablesProxyConfigurator.java:261:
private final Semaphore completionNotify = new Semaphore(1);
On 2014/11/24 13:07:57, René Jeschke wrote:
> On 2014/11/19 10:38:39, Felix H. Dahlke wrote:
> > This confused me a little bit, being a verb (bit unusual for field names).
How
> > about just "running" or something?
> 
> "running" as name for a flag would mean sense, but I'm using a Semaphore here,
> which is used for waiting/counting.
> 
> I will come up with something different, still.

Ok, I changed this to running as I don't have a better name (except
'semaphore').

Thing is, 'running' is wrong, as running is either true or false. A semaphore
has no true/false it is an integer.

Still, maybe someone finds a better name in the future^^

[Felix Dahlke, 2015-01-27 08:45:36]

LGTM

http://codereview.adblockplus.org/5624117646589952/diff/5685265389584384/src/...
File src/org/adblockplus/android/configurators/IptablesProxyConfigurator.java
(right):

http://codereview.adblockplus.org/5624117646589952/diff/5685265389584384/src/...
src/org/adblockplus/android/configurators/IptablesProxyConfigurator.java:261:
private final Semaphore completionNotify = new Semaphore(1);
On 2015/01/23 13:37:36, René Jeschke wrote:
> On 2014/11/24 13:07:57, René Jeschke wrote:
> > On 2014/11/19 10:38:39, Felix H. Dahlke wrote:
> > > This confused me a little bit, being a verb (bit unusual for field names).
> How
> > > about just "running" or something?
> > 
> > "running" as name for a flag would mean sense, but I'm using a Semaphore
here,
> > which is used for waiting/counting.
> > 
> > I will come up with something different, still.
> 
> Ok, I changed this to running as I don't have a better name (except
> 'semaphore').
> 
> Thing is, 'running' is wrong, as running is either true or false. A semaphore
> has no true/false it is an integer.
> 
> Still, maybe someone finds a better name in the future^^

True, but I figure for a semaphore with one permit, it seems OK.