Issue 8484110: ABP/Android proxy service (Closed)
|
Created:
Oct. 5, 2012, 9:28 a.m. by Andrey Novikov Modified:
Nov. 27, 2012, 10:51 a.m. Visibility:
Public. |
DescriptionABP/Android proxy service
Patch Set 1 #Patch Set 2 : ABP/Android proxy service #
Total comments: 76
Patch Set 3 : ABP/Android proxy service #
Total comments: 4
Patch Set 4 : ABP/Android proxy service #
Total comments: 1
MessagesTotal messages: 12
Here are my comments for ProxyService.java, I'll try and review the rest tomorrow morning. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... File src/org/adblockplus/android/ProxyService.java (right): http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:86: public void onCreate() It's a long method, you know what I think about those :) http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:110: if (Build.VERSION.SDK_INT >= 12) // Honeycomb 3.1 I've seen this elsewhere, can we use the same method/variable here? http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:118: String[] px = ProxySettings.getUserProxy(getApplicationContext()); // not used but left for future reference Future reference? It is in fact used below, if only for logging. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:136: iptables = getIptables(); If I understand it correctly, ABP can't be active if iptables isn't found, right? In that case I'd like some logging in case it's null, if just for debugging purposes. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:137: if (iptables != null) What if iptables is null? I'd like some logging, should make debugging easier. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:152: e.printStackTrace(); Here and below, I'd prefer Log.e(). http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:271: e.printStackTrace(); Log.e()? http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:404: if (Build.VERSION.SDK_INT < 12) // Honeycomb 3.1 Like before, I think there's a global variable or method indicating this. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:406: if (key.equals(getString(R.string.pref_proxyhost)) || key.equals(getString(R.string.pref_proxyport)) || key.equals(getString(R.string.pref_proxyuser)) I think you should call getString() just once for each pref key, it's called again below. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:424: return isTransparent; I believe the bean conventions says that boolean getter methods should be named isVariable and the field just variable. Just some nit picking of course. The same would go for isNativeProxy. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:442: if (host != null) This check is not necessary, host cannot be null at this point. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:460: { Comment or logging please. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:468: public String getIptables() throws IOException, RootToolsException, TimeoutException Bean convention again: Can you think of another prefix than "get"? Especially since it actually modifies the filesystem. How about something like "findIptablesExecutable()"? "find" is conventionally used for retrieval methods that could return null. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:476: return null; I think this is worth some logging, would help debugging. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:480: RootTools.sendShell("chmod 700 " + path, DEFAULT_TIMEOUT); "chmod +x" would be less intrusive. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:497: return null; Same here, logging? http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:523: * true if notification message should me set to normal operating should "be" set? http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:623: ProxySettings.setConnectionProxy(getApplicationContext(), "127.0.0.1", port, ""); I've seen "127.0.0.1" at least once before, maybe worth a constant? http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:651: e.printStackTrace(); Log.e()? http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:670: { Logging or a comment?
Some more comments. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... File src/org/adblockplus/brazil/TransparentProxyHandler.java (right): http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/TransparentProxyHandler.java:25: if (!request.url.contains("://")) Since we're only dealing with HTTP(S), something like this would be less ambiguous: request.url.matches("^http?s://")
Review done on my side. Haven't really found anything that would trigger alarm bells. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... File src/org/adblockplus/brazil/RequestHandler.java (right): http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/RequestHandler.java:80: private boolean shouldLog; // if true, log all headers Maybe call it shouldLogHeaders then? That'd make the comment redundant. I'm not a fan of comments where naming suffices, I've seen them get invalid over time way too often. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/RequestHandler.java:98: { It seems this case occurs when the port is not set, so it's not an error worth logging. I'd like a small comment like "// Use the default port" though. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/RequestHandler.java:123: { Can we really safely go on in this case? At least worth a comment IMO. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/RequestHandler.java:144: if (request.url.startsWith("http:") == false && request.url.startsWith("https:") == false) How about if (!request.url.matches("^https?:")) ? http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/RequestHandler.java:291: Log.e(prefix, request.url); Log.d()? http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/RequestHandler.java:338: ((ChunkedOutputStream) out).writeFinalChunk(); Why use instanceof? That would avoid class cast exceptions. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... File src/org/adblockplus/brazil/SSLConnectionHandler.java (right): http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/SSLConnectionHandler.java:51: public static final String PROXY_HOST = "proxyHost"; The indentation level in this file seems to be off. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/SSLConnectionHandler.java:76: { As in RequestHandler.init(), a comment would be nice. Actually, I think there is some potential for reducing duplication here. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/SSLConnectionHandler.java:192: e.printStackTrace(); Log.e()?
http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... File src/org/adblockplus/android/ProxyService.java (right): http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:86: public void onCreate() On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > It's a long method, you know what I think about those :) Yes, comments are introduced for your comfort. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:110: if (Build.VERSION.SDK_INT >= 12) // Honeycomb 3.1 On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > I've seen this elsewhere, can we use the same method/variable here? What exactly are you referencing? http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:118: String[] px = ProxySettings.getUserProxy(getApplicationContext()); // not used but left for future reference On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > Future reference? It is in fact used below, if only for logging. Yes, only for logging. Didn't get enough statistics on various devices to remove it. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:136: iptables = getIptables(); On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > If I understand it correctly, ABP can't be active if iptables isn't found, > right? In that case I'd like some logging in case it's null, if just for > debugging purposes. Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:137: if (iptables != null) On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > What if iptables is null? I'd like some logging, should make debugging easier. Then it just will not enable rooted mode and work as not rooted. It will be clearly seen in configuration settings output. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:152: e.printStackTrace(); On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > Here and below, I'd prefer Log.e(). Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:271: e.printStackTrace(); On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > Log.e()? Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:404: if (Build.VERSION.SDK_INT < 12) // Honeycomb 3.1 On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > Like before, I think there's a global variable or method indicating this. Build.VERSION.SDK_INT is global static variable, why do you want to encapsulate this? http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:406: if (key.equals(getString(R.string.pref_proxyhost)) || key.equals(getString(R.string.pref_proxyport)) || key.equals(getString(R.string.pref_proxyuser)) On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > I think you should call getString() just once for each pref key, it's called > again below. Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:424: return isTransparent; On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > I believe the bean conventions says that boolean getter methods should be named > isVariable and the field just variable. Just some nit picking of course. The > same would go for isNativeProxy. Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:442: if (host != null) On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > This check is not necessary, host cannot be null at this point. Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:460: { On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > Comment or logging please. Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:468: public String getIptables() throws IOException, RootToolsException, TimeoutException On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > Bean convention again: Can you think of another prefix than "get"? Especially > since it actually modifies the filesystem. How about something like > "findIptablesExecutable()"? "find" is conventionally used for retrieval methods > that could return null. It does not search for it - it's in the assets. It just initializes it, returning null if something goes wrong. May be I should refactor to use Exception for that? http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:476: return null; On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > I think this is worth some logging, would help debugging. Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:480: RootTools.sendShell("chmod 700 " + path, DEFAULT_TIMEOUT); On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > "chmod +x" would be less intrusive. We do not want any others to be able to execute it. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:497: return null; On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > Same here, logging? Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:523: * true if notification message should me set to normal operating On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > should "be" set? Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:623: ProxySettings.setConnectionProxy(getApplicationContext(), "127.0.0.1", port, ""); On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > I've seen "127.0.0.1" at least once before, maybe worth a constant? Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:651: e.printStackTrace(); On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > Log.e()? Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:670: { On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > Logging or a comment? No, it's on purpose - it occurs on each server stop. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... File src/org/adblockplus/brazil/RequestHandler.java (right): http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/RequestHandler.java:80: private boolean shouldLog; // if true, log all headers On 2012/11/09 06:04:32, Felix H. Dahlke wrote: > Maybe call it shouldLogHeaders then? That'd make the comment redundant. I'm not > a fan of comments where naming suffices, I've seen them get invalid over time > way too often. Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/RequestHandler.java:98: { On 2012/11/09 06:04:32, Felix H. Dahlke wrote: > It seems this case occurs when the port is not set, so it's not an error worth > logging. I'd like a small comment like "// Use the default port" though. Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/RequestHandler.java:123: { On 2012/11/09 06:04:32, Felix H. Dahlke wrote: > Can we really safely go on in this case? At least worth a comment IMO. We are transparent, it's not our deal if it's malformed. Just have to catch because of Java API. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/RequestHandler.java:144: if (request.url.startsWith("http:") == false && request.url.startsWith("https:") == false) On 2012/11/09 06:04:32, Felix H. Dahlke wrote: > How about if (!request.url.matches("^https?:")) ? I thought it would be faster... http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/RequestHandler.java:291: Log.e(prefix, request.url); On 2012/11/09 06:04:32, Felix H. Dahlke wrote: > Log.d()? Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/RequestHandler.java:338: ((ChunkedOutputStream) out).writeFinalChunk(); On 2012/11/09 06:04:32, Felix H. Dahlke wrote: > Why use instanceof? That would avoid class cast exceptions. Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... File src/org/adblockplus/brazil/SSLConnectionHandler.java (right): http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/SSLConnectionHandler.java:51: public static final String PROXY_HOST = "proxyHost"; On 2012/11/09 06:04:32, Felix H. Dahlke wrote: > The indentation level in this file seems to be off. I will fix it after review. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/SSLConnectionHandler.java:192: e.printStackTrace(); On 2012/11/09 06:04:32, Felix H. Dahlke wrote: > Log.e()? No, should remove Log.e above. This is not Android aware object. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... File src/org/adblockplus/brazil/TransparentProxyHandler.java (right): http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/TransparentProxyHandler.java:25: if (!request.url.contains("://")) On 2012/11/08 20:00:43, Felix H. Dahlke wrote: > Since we're only dealing with HTTP(S), something like this would be less > ambiguous: request.url.matches("^http?s://") Why are we? It's universal, it rewrites: GET /file.name into: GET http://host/file.name It should not do any rewriting in case it will find any schema in URL. I do not see any reason to break this universalism.
Reviewed your changes and addressed your comments. Only a few issues left. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... File src/org/adblockplus/android/ProxyService.java (right): http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:110: if (Build.VERSION.SDK_INT >= 12) // Honeycomb 3.1 On 2012/11/09 09:23:47, Andrey Novikov wrote: > On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > > I've seen this elsewhere, can we use the same method/variable here? > > What exactly are you referencing? I think it was AdvancedPreferences.hasNativeProxy. If the >= 3.1 checks are always used to figure out if that's the case, I think this check should only be done once, and set a central hasNativeProxy variable or something like that. What if the condition for native proxy support is going to become more complicated, e.g. because support is removed in a future version? We'll have to find all >= 3.1 checks in the code and hope that they were to test for native proxy support. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:118: String[] px = ProxySettings.getUserProxy(getApplicationContext()); // not used but left for future reference On 2012/11/09 09:23:47, Andrey Novikov wrote: > On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > > Future reference? It is in fact used below, if only for logging. > Yes, only for logging. Didn't get enough statistics on various devices to remove > it. I see, I misunderstood that then. I thought you meant reference somewhere in the source code. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:136: iptables = getIptables(); On 2012/11/09 09:23:47, Andrey Novikov wrote: > On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > > If I understand it correctly, ABP can't be active if iptables isn't found, > > right? In that case I'd like some logging in case it's null, if just for > > debugging purposes. > > Done. I see no changes. But since you explained that this is a perfectly valid case below, we probably don't need logging. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:404: if (Build.VERSION.SDK_INT < 12) // Honeycomb 3.1 On 2012/11/09 09:23:47, Andrey Novikov wrote: > On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > > Like before, I think there's a global variable or method indicating this. > > Build.VERSION.SDK_INT is global static variable, why do you want to encapsulate > this? See my reasoning above. If we're doing this check always for the same purpose (which I suspect), it should be done just once, and stored in an appropriately named variable (or done multiple times, but by an appropriately named method, I don't mind) http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:468: public String getIptables() throws IOException, RootToolsException, TimeoutException On 2012/11/09 09:23:47, Andrey Novikov wrote: > On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > > Bean convention again: Can you think of another prefix than "get"? Especially > > since it actually modifies the filesystem. How about something like > > "findIptablesExecutable()"? "find" is conventionally used for retrieval > methods > > that could return null. > > It does not search for it - it's in the assets. It just initializes it, > returning null if something goes wrong. May be I should refactor to use > Exception for that? > Oh, I thought it takes it from the system. How about "initIptables()" then? You could set the "iptables" field in here, no need to return it. Using exceptions sounds like a good idea. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:480: RootTools.sendShell("chmod 700 " + path, DEFAULT_TIMEOUT); On 2012/11/09 09:23:47, Andrey Novikov wrote: > On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > > "chmod +x" would be less intrusive. > > We do not want any others to be able to execute it. See above, I thought it was using a system wide executable. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:670: { On 2012/11/09 09:23:47, Andrey Novikov wrote: > On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > > Logging or a comment? > > No, it's on purpose - it occurs on each server stop. You could at least add a comment then, empty catch blocks always raise my alarm bells. The Android code guidelines are with me on this: http://source.android.com/source/code-style.html#dont-ignore-exceptions http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... File src/org/adblockplus/brazil/RequestHandler.java (right): http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/RequestHandler.java:123: { On 2012/11/09 09:23:47, Andrey Novikov wrote: > On 2012/11/09 06:04:32, Felix H. Dahlke wrote: > > Can we really safely go on in this case? At least worth a comment IMO. > > We are transparent, it's not our deal if it's malformed. Just have to catch > because of Java API. Hm, but should we just go on with a null reqHost and/or refHost? http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/RequestHandler.java:144: if (request.url.startsWith("http:") == false && request.url.startsWith("https:") == false) On 2012/11/09 09:23:47, Andrey Novikov wrote: > On 2012/11/09 06:04:32, Felix H. Dahlke wrote: > > How about if (!request.url.matches("^https?:")) ? > > I thought it would be faster... Premature optimisation? :) I think you should go with whatever you find most readable, unless there is an actual issue. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... File src/org/adblockplus/brazil/SSLConnectionHandler.java (right): http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/SSLConnectionHandler.java:51: public static final String PROXY_HOST = "proxyHost"; On 2012/11/09 09:23:47, Andrey Novikov wrote: > On 2012/11/09 06:04:32, Felix H. Dahlke wrote: > > The indentation level in this file seems to be off. > > I will fix it after review. Thanks, it would ruin the diffs :D http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/SSLConnectionHandler.java:192: e.printStackTrace(); On 2012/11/09 09:23:47, Andrey Novikov wrote: > On 2012/11/09 06:04:32, Felix H. Dahlke wrote: > > Log.e()? > > No, should remove Log.e above. This is not Android aware object. I see. http://codereview.adblockplus.org/8484110/diff/12001/src/org/adblockplus/braz... File src/org/adblockplus/brazil/RequestHandler.java (right): http://codereview.adblockplus.org/8484110/diff/12001/src/org/adblockplus/braz... src/org/adblockplus/brazil/RequestHandler.java:142: System.err.println(dumpHeaders(count, request, request.headers, true)); Haven't seen this code before, it was further down below before. Why System.err.println() and not my beloved Log.e()? :) http://codereview.adblockplus.org/8484110/diff/12001/src/org/adblockplus/braz... src/org/adblockplus/brazil/RequestHandler.java:147: request.sendHeaders(204, null, 0); This part is new, I presume it wasn't to address any review comments?
http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... File src/org/adblockplus/android/ProxyService.java (right): http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:404: if (Build.VERSION.SDK_INT < 12) // Honeycomb 3.1 On 2012/11/09 14:40:46, Felix H. Dahlke wrote: > On 2012/11/09 09:23:47, Andrey Novikov wrote: > > On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > > > Like before, I think there's a global variable or method indicating this. > > > > Build.VERSION.SDK_INT is global static variable, why do you want to > encapsulate > > this? > > See my reasoning above. If we're doing this check always for the same purpose > (which I suspect), it should be done just once, and stored in an appropriately > named variable (or done multiple times, but by an appropriately named method, I > don't mind) Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:468: public String getIptables() throws IOException, RootToolsException, TimeoutException On 2012/11/09 14:40:46, Felix H. Dahlke wrote: > On 2012/11/09 09:23:47, Andrey Novikov wrote: > > On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > > > Bean convention again: Can you think of another prefix than "get"? > Especially > > > since it actually modifies the filesystem. How about something like > > > "findIptablesExecutable()"? "find" is conventionally used for retrieval > > methods > > > that could return null. > > > > It does not search for it - it's in the assets. It just initializes it, > > returning null if something goes wrong. May be I should refactor to use > > Exception for that? > > > > Oh, I thought it takes it from the system. How about "initIptables()" then? You > could set the "iptables" field in here, no need to return it. > > Using exceptions sounds like a good idea. Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/andro... src/org/adblockplus/android/ProxyService.java:670: { On 2012/11/09 14:40:46, Felix H. Dahlke wrote: > On 2012/11/09 09:23:47, Andrey Novikov wrote: > > On 2012/11/08 16:37:50, Felix H. Dahlke wrote: > > > Logging or a comment? > > > > No, it's on purpose - it occurs on each server stop. > > You could at least add a comment then, empty catch blocks always raise my alarm > bells. The Android code guidelines are with me on this: > http://source.android.com/source/code-style.html#dont-ignore-exceptions Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... File src/org/adblockplus/brazil/RequestHandler.java (right): http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/RequestHandler.java:123: { In fact this exception is for referrer, request can hardly be malformed at this point, it should be processed earlier by third-party code. I can split it in two catches if you wish. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/RequestHandler.java:144: if (request.url.startsWith("http:") == false && request.url.startsWith("https:") == false) On 2012/11/09 14:40:46, Felix H. Dahlke wrote: > On 2012/11/09 09:23:47, Andrey Novikov wrote: > > On 2012/11/09 06:04:32, Felix H. Dahlke wrote: > > > How about if (!request.url.matches("^https?:")) ? > > > > I thought it would be faster... > > Premature optimisation? :) I think you should go with whatever you find most > readable, unless there is an actual issue. Done. http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... File src/org/adblockplus/brazil/SSLConnectionHandler.java (right): http://codereview.adblockplus.org/8484110/diff/3001/src/org/adblockplus/brazi... src/org/adblockplus/brazil/SSLConnectionHandler.java:76: { On 2012/11/09 06:04:32, Felix H. Dahlke wrote: > As in RequestHandler.init(), a comment would be nice. Actually, I think there is > some potential for reducing duplication here. Done. http://codereview.adblockplus.org/8484110/diff/12001/src/org/adblockplus/braz... File src/org/adblockplus/brazil/RequestHandler.java (right): http://codereview.adblockplus.org/8484110/diff/12001/src/org/adblockplus/braz... src/org/adblockplus/brazil/RequestHandler.java:142: System.err.println(dumpHeaders(count, request, request.headers, true)); On 2012/11/09 14:40:46, Felix H. Dahlke wrote: > Haven't seen this code before, it was further down below before. Why > System.err.println() and not my beloved Log.e()? :) For the same reason. It's not Android specific domain, I will have to remove all Log.* in fact. http://codereview.adblockplus.org/8484110/diff/12001/src/org/adblockplus/braz... src/org/adblockplus/brazil/RequestHandler.java:147: request.sendHeaders(204, null, 0); On 2012/11/09 14:40:46, Felix H. Dahlke wrote: > This part is new, I presume it wasn't to address any review comments? No, it's a fix I introduced. I can not wait for reviews so long. Sorry.
LGTM, if you fix the indentation issues. http://codereview.adblockplus.org/8484110/diff/20001/src/org/adblockplus/braz... File src/org/adblockplus/brazil/BaseRequestHandler.java (right): http://codereview.adblockplus.org/8484110/diff/20001/src/org/adblockplus/braz... src/org/adblockplus/brazil/BaseRequestHandler.java:23: this.prefix = prefix; Indentation is off here. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
