Issue 8478117: ABP/Android core (Closed)
|
Created:
Oct. 5, 2012, 9:29 a.m. by Andrey Novikov Modified:
Nov. 27, 2012, 10:51 a.m. Visibility:
Public. |
DescriptionABP/Android core
Patch Set 1 #
Total comments: 69
Patch Set 2 : ABP/Android core #MessagesTotal messages: 9
I'm done with my part of the review. Wladimir, do you want to check my issues first because Andrey addresses them? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... File src/org/adblockplus/android/AdblockPlus.java (right): http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:82: private static AdblockPlus myself; Nit: Isn't "instance" a more typical name for a singleton instance field? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:133: catch (ParserConfigurationException e) Can we log the following exceptions as an error instead of using printStackTrace()? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:179: * Subscription test = new Subscription(); Can we remove this commented code? It's in source control should we need it in the future. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:206: // TODO Auto-generated catch block Can we log this exception as an error instead of using printStackTrace()? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:292: // TODO Auto-generated catch block Like above, log an error instead? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:310: Future<String> future = js.submit(new Callable<String>() { Is the future really necessary here? It seems you just submit it and then wait for it, can't you call js.evaluate() directly with pretty much the same results? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:324: // TODO Auto-generated catch block Like above, error logging. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:355: Boolean result = (Boolean) js.evaluate("matchesAny('" + url + "', '" + query + "', '" + reqHost + "', '" + refHost + "', '" + accept + "');"); Nit: You could just return this directly http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:440: Log.e(TAG, "startEngine"); Shouldn't this be Log.d()? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:445: final int refresh = Integer.valueOf(prefs.getString(getString(R.string.pref_refresh), Integer.toString(getResources().getInteger(R.integer.def_refresh)))); We should use a fixed string for storing these preferences, not locale dependent ones. Otherwise I'd loose all preferences when switching languages, right? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:472: e.printStackTrace(); As before, error logging? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:538: Log.e(TAG, "Set updater alarm"); I guess Log.d() is more appropriate here? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:577: // TODO Auto-generated catch block Log.e()? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:642: e.printStackTrace(); Log.e()? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:650: Log.e(TAG, path); Log.d()? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:658: e.printStackTrace(); Log.e()? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:666: Log.e(TAG, path); Log.d()? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:674: e.printStackTrace(); Log.e()? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:708: Log.e(TAG, "httpSend('" + method + "', '" + url + "')"); Log.d()? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:740: @SuppressWarnings("unused") I don't think this is necessary http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:747: @SuppressWarnings("unused") I don't think this is necessary http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:751: Message msg = messageHandler.obtainMessage(MSG_TOAST); Log.d()? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:759: @SuppressWarnings("unused") I don't think this is necessary http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:823: e.printStackTrace(); Log.e()? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:849: { Log.e()? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:867: { Log.e()? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:874: e.printStackTrace(); Log.e()? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:968: Log.w("D", "S: " + lenghtOfFile); Log.d()? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:980: final char[] buffer = new char[0x10000]; Reminds me of the code up there, maybe have a single method for reading from a reader into a string? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:1000: e.printStackTrace(); Log.e()?
http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... File src/org/adblockplus/android/AdblockPlus.java (right): http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:82: private static AdblockPlus myself; On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Nit: Isn't "instance" a more typical name for a singleton instance field? Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:133: catch (ParserConfigurationException e) On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Can we log the following exceptions as an error instead of using > printStackTrace()? printStackTrace does pretty much the same, I use it to distinguish the parts, where I have decided on how to deal with an error and where I have left it as-is. But if you think that it's not good, I'll change it. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:179: * Subscription test = new Subscription(); On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Can we remove this commented code? It's in source control should we need it in > the future. Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:310: Future<String> future = js.submit(new Callable<String>() { On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Is the future really necessary here? It seems you just submit it and then wait > for it, can't you call js.evaluate() directly with pretty much the same results? No, I can't. JS runs in separate asynchronous thread, I can't access it directly. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:355: Boolean result = (Boolean) js.evaluate("matchesAny('" + url + "', '" + query + "', '" + reqHost + "', '" + refHost + "', '" + accept + "');"); On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Nit: You could just return this directly See above. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:440: Log.e(TAG, "startEngine"); On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Shouldn't this be Log.d()? I use logging not only for error reporting but also as a debug aid. This is why I use log high levels to emphasize major execution flow points. It helps much when I debug a lot. If you consider this bad style I will change log levels to appropriate ones. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:445: final int refresh = Integer.valueOf(prefs.getString(getString(R.string.pref_refresh), Integer.toString(getResources().getInteger(R.integer.def_refresh)))); On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > We should use a fixed string for storing these preferences, not locale dependent > ones. Otherwise I'd loose all preferences when switching languages, right? These are not locale dependent. Locale dependent strings are put in separate file and only they will be exposed for localization. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:740: @SuppressWarnings("unused") On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > I don't think this is necessary This is necessary because Eclipse highlights all warning and error points in source code, I do not want correct code to be highlighted and hinder locating real problems. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:980: final char[] buffer = new char[0x10000]; On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Reminds me of the code up there, maybe have a single method for reading from a > reader into a string? No, I can't. At least do not know how. Because here it can be canceled by user and publishes progress to UI.
I have responded. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... File src/org/adblockplus/android/AdblockPlus.java (right): http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:133: catch (ParserConfigurationException e) On 2012/10/30 07:53:22, Andrey Novikov wrote: > On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > > Can we log the following exceptions as an error instead of using > > printStackTrace()? > > printStackTrace does pretty much the same, I use it to distinguish the parts, > where I have decided on how to deal with an error and where I have left it > as-is. But if you think that it's not good, I'll change it. I think Log.e() is the canonical way to log exceptions in Android. Aren't we using quite a bit of extra information in logcat with printStackTrace()? I believe it doesn't show application, activity etc. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:310: Future<String> future = js.submit(new Callable<String>() { On 2012/10/30 07:53:22, Andrey Novikov wrote: > On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > > Is the future really necessary here? It seems you just submit it and then wait > > for it, can't you call js.evaluate() directly with pretty much the same > results? > > No, I can't. JS runs in separate asynchronous thread, I can't access it > directly. Oh, I see. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:440: Log.e(TAG, "startEngine"); On 2012/10/30 07:53:22, Andrey Novikov wrote: > On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > > Shouldn't this be Log.d()? > > I use logging not only for error reporting but also as a debug aid. This is why > I use log high levels to emphasize major execution flow points. It helps much > when I debug a lot. If you consider this bad style I will change log levels to > appropriate ones. That's fine for debugging of course, but I think we should either remove them or turn them into Log.d() if they should be permanent before committing. I don't know, seems strange to have errors that aren't errors in the log. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:445: final int refresh = Integer.valueOf(prefs.getString(getString(R.string.pref_refresh), Integer.toString(getResources().getInteger(R.integer.def_refresh)))); On 2012/10/30 07:53:22, Andrey Novikov wrote: > On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > > We should use a fixed string for storing these preferences, not locale > dependent > > ones. Otherwise I'd loose all preferences when switching languages, right? > > These are not locale dependent. Locale dependent strings are put in separate > file and only they will be exposed for localization. The string "pref_refresh" is "Subscription refresh". Are you sure that doesn't have to be translated? Why not use the string literal "refresh" for the preference instead? That's how the official examples do it: http://developer.android.com/guide/topics/data/data-storage.html#pref http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:740: @SuppressWarnings("unused") On 2012/10/30 07:53:22, Andrey Novikov wrote: > On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > > I don't think this is necessary > > This is necessary because Eclipse highlights all warning and error points in > source code, I do not want correct code to be highlighted and hinder locating > real problems. I know what it's for, but I can't see any unused variables in this method. Same below.
http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... File src/org/adblockplus/android/AdblockPlus.java (right): http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:206: // TODO Auto-generated catch block On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Can we log this exception as an error instead of using printStackTrace()? Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:292: // TODO Auto-generated catch block On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Like above, log an error instead? Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:324: // TODO Auto-generated catch block On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Like above, error logging. Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:440: Log.e(TAG, "startEngine"); On 2012/10/30 08:08:42, Felix H. Dahlke wrote: > On 2012/10/30 07:53:22, Andrey Novikov wrote: > > On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > > > Shouldn't this be Log.d()? > > > > I use logging not only for error reporting but also as a debug aid. This is > why > > I use log high levels to emphasize major execution flow points. It helps much > > when I debug a lot. If you consider this bad style I will change log levels to > > appropriate ones. > > That's fine for debugging of course, but I think we should either remove them or > turn them into Log.d() if they should be permanent before committing. I don't > know, seems strange to have errors that aren't errors in the log. Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:445: final int refresh = Integer.valueOf(prefs.getString(getString(R.string.pref_refresh), Integer.toString(getResources().getInteger(R.integer.def_refresh)))); On 2012/10/30 08:08:42, Felix H. Dahlke wrote: > On 2012/10/30 07:53:22, Andrey Novikov wrote: > > On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > > > We should use a fixed string for storing these preferences, not locale > > dependent > > > ones. Otherwise I'd loose all preferences when switching languages, right? > > > > These are not locale dependent. Locale dependent strings are put in separate > > file and only they will be exposed for localization. > > The string "pref_refresh" is "Subscription refresh". Are you sure that doesn't > have to be translated? Why not use the string literal "refresh" for the > preference instead? That's how the official examples do it: > > http://developer.android.com/guide/topics/data/data-storage.html#pref No, the string pref_refresh is "refresh", and what you mean is pref_refresh_title. Yes, it's how examples are written, for simplicity, but it's a bad style: 1. You can mistype it in some other part of a code and result in dealing with another parameter. 2. There are situations when the parameter need to be renamed (back compatibility issues). But I can simplify things if you wish. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:472: e.printStackTrace(); On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > As before, error logging? Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:538: Log.e(TAG, "Set updater alarm"); On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > I guess Log.d() is more appropriate here? Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:577: // TODO Auto-generated catch block On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Log.e()? Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:642: e.printStackTrace(); On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Log.e()? Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:650: Log.e(TAG, path); On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Log.d()? Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:658: e.printStackTrace(); On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Log.e()? Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:666: Log.e(TAG, path); On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Log.d()? Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:674: e.printStackTrace(); On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Log.e()? Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:708: Log.e(TAG, "httpSend('" + method + "', '" + url + "')"); On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Log.d()? Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:740: @SuppressWarnings("unused") On 2012/10/30 08:08:42, Felix H. Dahlke wrote: > On 2012/10/30 07:53:22, Andrey Novikov wrote: > > On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > > > I don't think this is necessary > > > > This is necessary because Eclipse highlights all warning and error points in > > source code, I do not want correct code to be highlighted and hinder locating > > real problems. > > I know what it's for, but I can't see any unused variables in this method. Same > below. It's not about variables but about the method itself. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:751: Message msg = messageHandler.obtainMessage(MSG_TOAST); On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Log.d()? Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:823: e.printStackTrace(); On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Log.e()? Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:849: { On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Log.e()? Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:867: { On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Log.e()? Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:874: e.printStackTrace(); On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Log.e()? Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:968: Log.w("D", "S: " + lenghtOfFile); On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Log.d()? Done. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:1000: e.printStackTrace(); On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > Log.e()? Done.
Just one more comment on the preference string thing. http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... File src/org/adblockplus/android/AdblockPlus.java (right): http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:445: final int refresh = Integer.valueOf(prefs.getString(getString(R.string.pref_refresh), Integer.toString(getResources().getInteger(R.integer.def_refresh)))); On 2012/10/30 09:24:17, Andrey Novikov wrote: > On 2012/10/30 08:08:42, Felix H. Dahlke wrote: > > On 2012/10/30 07:53:22, Andrey Novikov wrote: > > > On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > > > > We should use a fixed string for storing these preferences, not locale > > > dependent > > > > ones. Otherwise I'd loose all preferences when switching languages, right? > > > > > > These are not locale dependent. Locale dependent strings are put in separate > > > file and only they will be exposed for localization. > > > > The string "pref_refresh" is "Subscription refresh". Are you sure that doesn't > > have to be translated? Why not use the string literal "refresh" for the > > preference instead? That's how the official examples do it: > > > > http://developer.android.com/guide/topics/data/data-storage.html#pref > > No, the string pref_refresh is "refresh", and what you mean is > pref_refresh_title. Yes, it's how examples are written, for simplicity, but it's > a bad style: > 1. You can mistype it in some other part of a code and result in dealing with > another parameter. > 2. There are situations when the parameter need to be renamed (back > compatibility issues). True, repeated string literals aren't great. How about constants though? http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:740: @SuppressWarnings("unused") On 2012/10/30 09:24:17, Andrey Novikov wrote: > On 2012/10/30 08:08:42, Felix H. Dahlke wrote: > > On 2012/10/30 07:53:22, Andrey Novikov wrote: > > > On 2012/10/29 15:06:36, Felix H. Dahlke wrote: > > > > I don't think this is necessary > > > > > > This is necessary because Eclipse highlights all warning and error points in > > > source code, I do not want correct code to be highlighted and hinder > locating > > > real problems. > > > > I know what it's for, but I can't see any unused variables in this method. > Same > > below. > > It's not about variables but about the method itself. Ah, I see. You only need that code for debugging?
I'd prefer using string constants for preferences instead of string resources. But it seems to be quite a big change, so I don't insist. LGTM from my side.
http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... File src/org/adblockplus/android/AdblockPlus.java (right): http://codereview.adblockplus.org/8478117/diff/1/src/org/adblockplus/android/... src/org/adblockplus/android/AdblockPlus.java:740: @SuppressWarnings("unused") No, it's called by JS engine from native code, that's why Eclipse thinks that it's not used. 
|
