Issue 4399 - Add WebView inheritor with ad blocking

Issue 4399 - Add WebView inheritor with ad blocking

This is patchset 20 for the code review https://codereview.adblockplus.org/29351744/

Important related ticket:
https://codereview.adblockplus.org/29361556

[diegocarloslima, 2016-11-08 16:30:59]

https://codereview.adblockplus.org/29361445/diff/29361446/.hgignore
File .hgignore (right):

https://codereview.adblockplus.org/29361445/diff/29361446/.hgignore#newcode3
.hgignore:3: **/ant.properties
In order to provide better maintainability, I would suggest to have all rules in
the very same order in both .gitignore and .hgignore

https://codereview.adblockplus.org/29361445/diff/29361446/.hgignore#newcode7
.hgignore:7: **/libs/x86
These two rules are slightly different from the ones in .gitignore: 
libadblockplus-android/libs/armeabi*
libadblockplus-android/libs/x86

I would advise to keep it consistent in both files.

https://codereview.adblockplus.org/29361445/diff/29361446/.hgignore#newcode15
.hgignore:15: .*
The rules !.gitignore and !.hgignore are missing here, otherwise they will be
ignored by the above rule

https://codereview.adblockplus.org/29361445/diff/29361446/.hgignore#newcode17
.hgignore:17: **/target
buildtools rule is missing here in comparison to .gitignore. Any particular
reason for that?

https://codereview.adblockplus.org/29361445/diff/29361446/README.md
File README.md (right):

https://codereview.adblockplus.org/29361445/diff/29361446/README.md#newcode184
README.md:184: -----------------
For me it's a bit weird that a subsection has a higher header hierarchy (h2)
than its parent (h3). Here, we can still deduce that 'Building with Ant' is
inside the context of 'Building', but with other titles that could lead to
confusion.

https://codereview.adblockplus.org/29361445/diff/29361446/README.md#newcode215
README.md:215: -----------------
Same here, 'Building with Ant' and 'Building with Gradle' have higher header
hierarchy than 'Building'

https://codereview.adblockplus.org/29361445/diff/29361446/README.md#newcode274
README.md:274: -----------------
Same here, 'Building with Ant' and 'Building with Gradle' have higher header
hierarchy than 'Building'

https://codereview.adblockplus.org/29361445/diff/29361446/README.md#newcode305
README.md:305: -----------------
Same here, 'Building with Ant' and 'Building with Gradle' have higher header
hierarchy than 'Building'

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-settings/AndroidManifest.xml (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/AndroidManifest.xml:9:
android:targetSdkVersion="21" />
Couldn't we target at least API 22?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-settings/build.gradle (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/build.gradle:9: targetSdkVersion 21
Same as I commented on AndroidManifest. Couldn't we target at least API 22?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-settings/pom.xml (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/pom.xml:25: <version>5.0</version> <!-- 5.0 =
target-21 -->
Couldn't we target at least API 22?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/pom.xml:50: <platform>21</platform>
Couldn't we target at least API 22?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-settings/project.properties (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/project.properties:12: target=android-21
Couldn't we target at least API 22?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File
libadblockplus-android-settings/res/layout/fragment_adblock_whitelisted_domain_item.xml
(right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/res/layout/fragment_adblock_whitelisted_domain_item.xml:15:
android:layout_width="1px"
It's usually recommended to use 0dp here when layout_weight is also present. Any
particular reason for using 1px?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File
libadblockplus-android-settings/res/layout/fragment_adblock_whitelisted_domains_settings.xml
(right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/res/layout/fragment_adblock_whitelisted_domains_settings.xml:19:
android:layout_weight="1"
It's usually recommended to use 0dp here when layout_weight is also present. Any
particular reason for using 1px?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/Adblock.java
(right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/Adblock.java:46:
}
private constructor would be more appropriate IMHO

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/Adblock.java:48:
private static Adblock _instance;
I believe that we don't name static members beginning with underscore. I
couldn't find any static member that wasn't final in our codebase to confirm it.
We should ask Felix or René about this.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/Adblock.java:75:
private AdblockSettingsStorage storage;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/Adblock.java:90:
this.context = context;
We should use this.context = context.getApplicationContext() here otherwise it
can be the context of an Activity, for example, and this will leak, since the
AdblockPlus instance is kept as a singleton

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/Adblock.java:139:
private AtomicInteger referenceCounter = new AtomicInteger(0);
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockGeneralSettingsFragment.java
(right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockGeneralSettingsFragment.java:22:
import android.preference.CheckBoxPreference;
This import is never used

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockGeneralSettingsFragment.java:47:
private String SETTINGS_WL_DOMAINS_KEY;
This naming convention should be applied only for constants

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockGeneralSettingsFragment.java:213:
Set<String> selectedValues = newValue;
Why don't we just use the newValue parameter instead of declaring another
variable?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettings.java
(right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettings.java:27:
import java.util.Set;
Arrays, HashSet, LinkedList and Set imports doesn't seem to be used

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettings.java:46:
private transient Boolean acceptableAdsEnabled;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettings.java:58:
private List<Subscription> subscriptions;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettings.java:70:
private List<String> whitelistedDomains;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettingsFragment.java
(right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettingsFragment.java:110:
protected AdblockSettings settings;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockWhitelistedDomainsSettingsFragment.java
(right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockWhitelistedDomainsSettingsFragment.java:81:
private Adapter adapter;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockWhitelistedDomainsSettingsFragment.java:185:
holder.removeButton.setTag(new Integer(position));
Integer.valueOf should be better here, since it can return Integer objects that
are in the objects pool, so it can be more efficient

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-webview/AndroidManifest.xml (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/AndroidManifest.xml:9:
android:targetSdkVersion="21" />
Couldn't we target at least API 22?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-webview/build.gradle (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/build.gradle:9: targetSdkVersion 21
Couldn't we target at least API 22?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-webview/pom.xml (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/pom.xml:25: <version>5.0</version> <!-- 5.0 =
target-21 -->
Couldn't we target at least API 22?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/pom.xml:50: <platform>21</platform>
Couldn't we target at least API 22?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-webview/project.properties (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/project.properties:12: target=android-21
Couldn't we target at least API 22?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java
(right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:56:
import java.util.Arrays;
ArrayList and Arrays import seems to not being used here

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:90:
private volatile boolean addDomListener = true;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:109:
private boolean adblockEnabled = true;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:128:
private WebChromeClient extWebChromeClient;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:137:
private boolean debugMode;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:178:

I would rather have these log methods in a utility class, that way we can reduce
a bit the size of this file

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:182:
private static final String BRIDGE = "jsBridge";
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:192:
private void runScript(String script)
We might should add @TargetApi annotation here, to avoid lint errors

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:206:
private AdblockEngine adblockEngine;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:214:
private Integer loadError;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:249:
private WebChromeClient intWebChromeClient = new WebChromeClient()
This annonymous inner class is really big, reducing the overall readability of
the file. It would be better if we move it to a separate file, so we can
significantly reduce the size of this file

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:558:
public static final int ALLOW_DRAW_DELAY = 200;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:564:
private int allowDrawDelay = ALLOW_DRAW_DELAY;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:586:
private WebViewClient extWebViewClient;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:599:
private static final Pattern RE_HTML = Pattern.compile("\\.html?$",
Pattern.CASE_INSENSITIVE);
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:601:
private WebViewClient intWebViewClient;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:607:
private class AdblockWebViewClient extends WebViewClient
This annonymous inner class is really big, reducing the overall readability of
the file. It would be better if we move it to a separate file, so we can
significantly reduce the size of this file

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:925:
private class AdblockWebViewClient21 extends AdblockWebViewClient
This annonymous inner class is really big, reducing the overall readability of
the file. It would be better if we move it to a separate file, so we can
significantly reduce the size of this file

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:957:
private Map<String, String> url2Referrer = Collections.synchronizedMap(new
HashMap<String, String>());
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:1054:
private ElemHideThread elemHideThread;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:1058:
};
I would prefer to go for EMPTY_ARRAY = {};

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:1137:
private Object finishedRunnableLockObject = new Object();
Lock objects for synchronized statements should be final

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:1170:
private boolean loading;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:1349:
private volatile boolean elementsHidden = false;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:1389:
private volatile boolean allowDraw = true;
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:1411:
private Handler handler = new Handler();
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:1437:
private static final String EMPTY_ELEMHIDE_ARRAY_STRING = "[]";
I feel like having member variable declarations mixed with methods is less
readable and confusing

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-webviewapp/AndroidManifest.xml (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webviewapp/AndroidManifest.xml:9:
android:targetSdkVersion="21"/>
Couldn't we target at least API 22?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webviewapp/AndroidManifest.xml:15:
<android:uses-permission
android:name="android.permission.READ_EXTERNAL_STORAGE"/>
These 'uses-permission' elements with android namespace looks weird, never saw
them before. Are you sure they shouldn't be just 'uses-permission' ?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-webviewapp/build.gradle (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webviewapp/build.gradle:22: targetSdkVersion 21
Couldn't we target at least API 22?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-webviewapp/pom.xml (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webviewapp/pom.xml:25: <version>5.0</version> <!-- 5.0 =
target-21 -->
Couldn't we target at least API 22?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webviewapp/pom.xml:58: <platform>21</platform>
Couldn't we target at least API 22?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-webviewapp/project.properties (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webviewapp/project.properties:11: target=android-21
Couldn't we target at least API 22?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-webviewapp/res/layout/activity_main.xml (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webviewapp/res/layout/activity_main.xml:13:
android:text="http://www.google.com"
I'm personally not a big fan of hardcoding strings in XML files. Even if it's on
the sample app

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webviewapp/res/layout/activity_main.xml:15:
android:singleLine="true"/>
singleLine is deprecated, it's better to use maxLines instead

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android/build.gradle (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android/build.gradle:9: targetSdkVersion 16
Couldn't we target at least API 22?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/android/Utils.java
(right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/Utils.java:25:
import java.net.URISyntaxException;
URI and URISyntaxException seems to not being used here

[anton, 2016-11-09 12:30:44]

Updated according to Diego's suggestions (patch set 3)

https://codereview.adblockplus.org/29361445/diff/29361446/.hgignore
File .hgignore (right):

https://codereview.adblockplus.org/29361445/diff/29361446/.hgignore#newcode3
.hgignore:3: **/ant.properties
On 2016/11/08 16:30:36, diegocarloslima wrote:
> In order to provide better maintainability, I would suggest to have all rules
in
> the very same order in both .gitignore and .hgignore

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29361446/.hgignore#newcode7
.hgignore:7: **/libs/x86
On 2016/11/08 16:30:36, diegocarloslima wrote:
> These two rules are slightly different from the ones in .gitignore: 
> libadblockplus-android/libs/armeabi*
> libadblockplus-android/libs/x86
> 
> I would advise to keep it consistent in both files.

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29361446/.hgignore#newcode15
.hgignore:15: .*
On 2016/11/08 16:30:36, diegocarloslima wrote:
> The rules !.gitignore and !.hgignore are missing here, otherwise they will be
> ignored by the above rule

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29361446/.hgignore#newcode17
.hgignore:17: **/target
On 2016/11/08 16:30:36, diegocarloslima wrote:
> buildtools rule is missing here in comparison to .gitignore. Any particular
> reason for that?

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29361446/README.md
File README.md (right):

https://codereview.adblockplus.org/29361445/diff/29361446/README.md#newcode184
README.md:184: -----------------
On 2016/11/08 16:30:37, diegocarloslima wrote:
> For me it's a bit weird that a subsection has a higher header hierarchy (h2)
> than its parent (h3). Here, we can still deduce that 'Building with Ant' is
> inside the context of 'Building', but with other titles that could lead to
> confusion.

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29361446/README.md#newcode215
README.md:215: -----------------
On 2016/11/08 16:30:37, diegocarloslima wrote:
> Same here, 'Building with Ant' and 'Building with Gradle' have higher header
> hierarchy than 'Building'

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29361446/README.md#newcode305
README.md:305: -----------------
On 2016/11/08 16:30:37, diegocarloslima wrote:
> Same here, 'Building with Ant' and 'Building with Gradle' have higher header
> hierarchy than 'Building'

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-settings/AndroidManifest.xml (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/AndroidManifest.xml:9:
android:targetSdkVersion="21" />
On 2016/11/08 16:30:38, diegocarloslima wrote:
> Couldn't we target at least API 22?

What's the point?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-settings/pom.xml (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/pom.xml:25: <version>5.0</version> <!-- 5.0 =
target-21 -->
On 2016/11/08 16:30:38, diegocarloslima wrote:
> Couldn't we target at least API 22?

What's the point?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/pom.xml:50: <platform>21</platform>
On 2016/11/08 16:30:38, diegocarloslima wrote:
> Couldn't we target at least API 22?

What's the point?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File
libadblockplus-android-settings/res/layout/fragment_adblock_whitelisted_domain_item.xml
(right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/res/layout/fragment_adblock_whitelisted_domain_item.xml:15:
android:layout_width="1px"
On 2016/11/08 16:30:39, diegocarloslima wrote:
> It's usually recommended to use 0dp here when layout_weight is also present.
Any
> particular reason for using 1px?

The reason was Android Studio was reporting that the element will be invisible
even if it has weight=1 (why?).
I used to use 0 too so i agree i should use 0 anyway.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File
libadblockplus-android-settings/res/layout/fragment_adblock_whitelisted_domains_settings.xml
(right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/res/layout/fragment_adblock_whitelisted_domains_settings.xml:19:
android:layout_weight="1"
On 2016/11/08 16:30:39, diegocarloslima wrote:
> It's usually recommended to use 0dp here when layout_weight is also present.
Any
> particular reason for using 1px?

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/Adblock.java
(right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/Adblock.java:46:
}
On 2016/11/08 16:30:40, diegocarloslima wrote:
> private constructor would be more appropriate IMHO

It depends. For now i think `protected` still works (prevents from
instantiating) but is not so restrictive as `private`.
Who knows how it will be used by the users and a library should not be too
restrictive without special need.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/Adblock.java:48:
private static Adblock _instance;
On 2016/11/08 16:30:40, diegocarloslima wrote:
> I believe that we don't name static members beginning with underscore. I
> couldn't find any static member that wasn't final in our codebase to confirm
it.
> We should ask Felix or René about this.

It's just my habit and i don't remember any special root about it in our code
style guide too

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/Adblock.java:90:
this.context = context;
On 2016/11/08 16:30:40, diegocarloslima wrote:
> We should use this.context = context.getApplicationContext() here otherwise it
> can be the context of an Activity, for example, and this will leak, since the
> AdblockPlus instance is kept as a singleton

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/Adblock.java:139:
private AtomicInteger referenceCounter = new AtomicInteger(0);
On 2016/11/08 16:30:40, diegocarloslima wrote:
> I feel like having member variable declarations mixed with methods is less
> readable and confusing

It's the result of 2 contradictory rules:
1) to hold variable in the beginning
2) to hold methods close to variable they access

It depends. Felix, Rene?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockGeneralSettingsFragment.java
(right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockGeneralSettingsFragment.java:22:
import android.preference.CheckBoxPreference;
On 2016/11/08 16:30:41, diegocarloslima wrote:
> This import is never used

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockGeneralSettingsFragment.java:47:
private String SETTINGS_WL_DOMAINS_KEY;
On 2016/11/08 16:30:41, diegocarloslima wrote:
> This naming convention should be applied only for constants

they can be considered as `final`.
The only reason they are not marked as `final` is they required context to
access string values so they are set in constructor and never changed later.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockGeneralSettingsFragment.java:213:
Set<String> selectedValues = newValue;
On 2016/11/08 16:30:41, diegocarloslima wrote:
> Why don't we just use the newValue parameter instead of declaring another
> variable?

Yes, we can and we should.
It's just because of Android Studio refactoring 'extractMethod' used.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettings.java
(right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettings.java:27:
import java.util.Set;
On 2016/11/08 16:30:42, diegocarloslima wrote:
> Arrays, HashSet, LinkedList and Set imports doesn't seem to be used

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettings.java:46:
private transient Boolean acceptableAdsEnabled;
On 2016/11/08 16:30:43, diegocarloslima wrote:
> I feel like having member variable declarations mixed with methods is less
> readable and confusing

I feel comfortable with both styles because of advanced IDE and i will do what
is more preferred to company style (did not find it in
http://www.oracle.com/technetwork/java/javase/documentation/codeconvtoc-13605...)

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettings.java:58:
private List<Subscription> subscriptions;
On 2016/11/08 16:30:42, diegocarloslima wrote:
> I feel like having member variable declarations mixed with methods is less
> readable and confusing

I feel comfortable with both styles because of advanced IDE and i will do what
is more preferred to company style (did not find it in
http://www.oracle.com/technetwork/java/javase/documentation/codeconvtoc-13605...)

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettings.java:70:
private List<String> whitelistedDomains;
On 2016/11/08 16:30:42, diegocarloslima wrote:
> I feel like having member variable declarations mixed with methods is less
> readable and confusing

I feel comfortable with both styles because of advanced IDE and i will do what
is more preferred to company style (did not find it in
http://www.oracle.com/technetwork/java/javase/documentation/codeconvtoc-13605...)

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettingsFragment.java
(right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettingsFragment.java:110:
protected AdblockSettings settings;
On 2016/11/08 16:30:43, diegocarloslima wrote:
> I feel like having member variable declarations mixed with methods is less
> readable and confusing

I feel comfortable with both styles because of advanced IDE and i will do what
is more preferred to company style (did not find it in
http://www.oracle.com/technetwork/java/javase/documentation/codeconvtoc-13605...)

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockWhitelistedDomainsSettingsFragment.java
(right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockWhitelistedDomainsSettingsFragment.java:81:
private Adapter adapter;
On 2016/11/08 16:30:43, diegocarloslima wrote:
> I feel like having member variable declarations mixed with methods is less
> readable and confusing

I feel comfortable with both styles because of advanced IDE and i will do what
is more preferred to company style (did not find it in
http://www.oracle.com/technetwork/java/javase/documentation/codeconvtoc-13605...)

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockWhitelistedDomainsSettingsFragment.java:185:
holder.removeButton.setTag(new Integer(position));
On 2016/11/08 16:30:43, diegocarloslima wrote:
> Integer.valueOf should be better here, since it can return Integer objects
that
> are in the objects pool, so it can be more efficient

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-webview/AndroidManifest.xml (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/AndroidManifest.xml:9:
android:targetSdkVersion="21" />
On 2016/11/08 16:30:44, diegocarloslima wrote:
> Couldn't we target at least API 22?

What's the point? I believe we should use the most ancient (21) or most recent
(as google suggests) 24.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-webview/pom.xml (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/pom.xml:25: <version>5.0</version> <!-- 5.0 =
target-21 -->
On 2016/11/08 16:30:44, diegocarloslima wrote:
> Couldn't we target at least API 22?

what's the point?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/pom.xml:50: <platform>21</platform>
On 2016/11/08 16:30:44, diegocarloslima wrote:
> Couldn't we target at least API 22?

what's the point?

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java
(right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:56:
import java.util.Arrays;
On 2016/11/08 16:30:46, diegocarloslima wrote:
> ArrayList and Arrays import seems to not being used here

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:178:

On 2016/11/08 16:30:54, diegocarloslima wrote:
> I would rather have these log methods in a utility class, that way we can
reduce
> a bit the size of this file

i'd prefer to use logging facade like slf4j but i don't want to introduce new
dependencies without special need

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:192:
private void runScript(String script)
On 2016/11/08 16:30:45, diegocarloslima wrote:
> We might should add @TargetApi annotation here, to avoid lint errors

It will work for android pre-19 too. If it works starting android 19 only, then
i agree.
For now leaving it as-is.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:249:
private WebChromeClient intWebChromeClient = new WebChromeClient()
On 2016/11/08 16:30:47, diegocarloslima wrote:
> This annonymous inner class is really big, reducing the overall readability of
> the file. It would be better if we move it to a separate file, so we can
> significantly reduce the size of this file

I've been thinking about it.
Unfortunately client and webview are coupled too much so it's undesired to have
2 separate files.
Also you can just wrap it in IDE.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:607:
private class AdblockWebViewClient extends WebViewClient
On 2016/11/08 16:30:45, diegocarloslima wrote:
> This annonymous inner class is really big, reducing the overall readability of
> the file. It would be better if we move it to a separate file, so we can
> significantly reduce the size of this file

same reason as for another client. Also they will never be used separately so
readability is the only reason and it can be solved by IDE gratefully.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:1058:
};
On 2016/11/08 16:30:53, diegocarloslima wrote:
> I would prefer to go for EMPTY_ARRAY = {};

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:1137:
private Object finishedRunnableLockObject = new Object();
On 2016/11/08 16:30:54, diegocarloslima wrote:
> Lock objects for synchronized statements should be final

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-webviewapp/AndroidManifest.xml (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webviewapp/AndroidManifest.xml:15:
<android:uses-permission
android:name="android.permission.READ_EXTERNAL_STORAGE"/>
On 2016/11/08 16:30:56, diegocarloslima wrote:
> These 'uses-permission' elements with android namespace looks weird, never saw
> them before. Are you sure they shouldn't be just 'uses-permission' ?

Agee, they were added by IDE and to be honest i don't remember why.
Also i'm not sure why READ_PHONE_STATE is required.
Fixed it.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
File libadblockplus-android-webviewapp/res/layout/activity_main.xml (right):

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webviewapp/res/layout/activity_main.xml:13:
android:text="http://www.google.com"
On 2016/11/08 16:30:57, diegocarloslima wrote:
> I'm personally not a big fan of hardcoding strings in XML files. Even if it's
on
> the sample app

It just for testing purposing, agree to remove it.

https://codereview.adblockplus.org/29361445/diff/29361446/libadblockplus-andr...
libadblockplus-android-webviewapp/res/layout/activity_main.xml:15:
android:singleLine="true"/>
On 2016/11/08 16:30:58, diegocarloslima wrote:
> singleLine is deprecated, it's better to use maxLines instead

I've seen IDE suggestion to replace it but it provides different behaviour (not
sure what was it, probably wrapping).
I will replace it.

[diegocarloslima, 2016-11-29 17:47:47]

Apart from the code comments, there are 2 more things that I would like to point
out:

- Although this reduces the size of the project, it isn't a good practice to put
the icon drawable on the default drawable folder

- Also, I'm not totally happy with the instance variables declaration mixed with
the methods. I would like to get a 2nd opinion about that.

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
File
libadblockplus-android-settings/res/layout/fragment_adblock_whitelisted_domain_item.xml
(right):

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
libadblockplus-android-settings/res/layout/fragment_adblock_whitelisted_domain_item.xml:15:
android:layout_width="0px"
You have changed here to '0px' while in
'fragment_adblock_whitelisted_domains_settings', you changed to '0dp'. To keep
consistency, I would advise to change here to '0dp' as well

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockGeneralSettingsFragment.java
(right):

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockGeneralSettingsFragment.java:150:
Set<String> selectedSubscriptionValues = new HashSet<String>();
I wouldn't insist, but I would advise to use the diamond operator here: 'new
HashSet<>()'

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockGeneralSettingsFragment.java:212:
List<Subscription> selectedSubscriptions = new LinkedList<Subscription>();
I wouldn't insist, but I would advise to use the diamond operator here: 'new
LinkedList<>()'

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettingsSharedPrefsStorage.java
(right):

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettingsSharedPrefsStorage.java:92:
List<String> whitelistedDomains = new LinkedList<String>();
I wouldn't insist, but I would advise to use the diamond operator here: 'new
LinkedList<>()'

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettingsSharedPrefsStorage.java:110:
List<Subscription> subscriptions = new LinkedList<Subscription>();
I wouldn't insist, but I would advise to use the diamond operator here: 'new
LinkedList<>()'

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettingsStorage.java
(right):

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettingsStorage.java:63:
List<Subscription> subscriptionsWithoutAA = new LinkedList<Subscription>();
I wouldn't insist, but I would advise to use the diamond operator here: 'new
LinkedList<>()'

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockWhitelistedDomainsSettingsFragment.java
(right):

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockWhitelistedDomainsSettingsFragment.java:230:
whitelistedDomains = new LinkedList<String>();
I wouldn't insist, but I would advise to use the diamond operator here: 'new
LinkedList<>()'

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
File
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java
(right):

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:1071:
private Object elemHideThreadLockObject = new Object();
Lock objects for synchronized statements should be final

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
(right):

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:77:
this.context = context;
This class receives a context by it's constructor, but the it seems to be never
used. It's advised to avoid unnecessary references to Context to minimize
leaking chances.

[anton, 2016-11-30 06:34:46]

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
File
libadblockplus-android-settings/res/layout/fragment_adblock_whitelisted_domain_item.xml
(right):

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
libadblockplus-android-settings/res/layout/fragment_adblock_whitelisted_domain_item.xml:15:
android:layout_width="0px"
On 2016/11/29 17:47:40, diegocarloslima wrote:
> You have changed here to '0px' while in
> 'fragment_adblock_whitelisted_domains_settings', you changed to '0dp'. To keep
> consistency, I would advise to change here to '0dp' as well

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockGeneralSettingsFragment.java
(right):

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockGeneralSettingsFragment.java:150:
Set<String> selectedSubscriptionValues = new HashSet<String>();
On 2016/11/29 17:47:40, diegocarloslima wrote:
> I wouldn't insist, but I would advise to use the diamond operator here: 'new
> HashSet<>()'

Diamond operator is supported starting Java 7 and i'd prefer back compatibility
to beauty.
It's a matter of agreement what version of Java to be used.

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockGeneralSettingsFragment.java:212:
List<Subscription> selectedSubscriptions = new LinkedList<Subscription>();
On 2016/11/29 17:47:41, diegocarloslima wrote:
> I wouldn't insist, but I would advise to use the diamond operator here: 'new
> LinkedList<>()'

Same here (see above)

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettingsSharedPrefsStorage.java
(right):

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettingsSharedPrefsStorage.java:92:
List<String> whitelistedDomains = new LinkedList<String>();
On 2016/11/29 17:47:42, diegocarloslima wrote:
> I wouldn't insist, but I would advise to use the diamond operator here: 'new
> LinkedList<>()'

Diamond operator is supported starting Java 7 and i'd prefer back compatibility
to beauty.
It's a matter of agreement what version of Java to be used.

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettingsSharedPrefsStorage.java:110:
List<Subscription> subscriptions = new LinkedList<Subscription>();
On 2016/11/29 17:47:41, diegocarloslima wrote:
> I wouldn't insist, but I would advise to use the diamond operator here: 'new
> LinkedList<>()'

Diamond operator is supported starting Java 7 and i'd prefer back compatibility
to beauty.
It's a matter of agreement what version of Java to be used.

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockWhitelistedDomainsSettingsFragment.java
(right):

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockWhitelistedDomainsSettingsFragment.java:230:
whitelistedDomains = new LinkedList<String>();
On 2016/11/29 17:47:44, diegocarloslima wrote:
> I wouldn't insist, but I would advise to use the diamond operator here: 'new
> LinkedList<>()'

Diamond operator is supported starting Java 7 and i'd prefer back compatibility
to beauty.
It's a matter of agreement what version of Java to be used.

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
File
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java
(right):

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:1071:
private Object elemHideThreadLockObject = new Object();
On 2016/11/29 17:47:45, diegocarloslima wrote:
> Lock objects for synchronized statements should be final

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
(right):

https://codereview.adblockplus.org/29361445/diff/29364268/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:77:
this.context = context;
On 2016/11/29 17:47:45, diegocarloslima wrote:
> This class receives a context by it's constructor, but the it seems to be
never
> used. It's advised to avoid unnecessary references to Context to minimize
> leaking chances.

Acknowledged.

[diegocarloslima, 2016-12-15 14:42:10]

https://codereview.adblockplus.org/29361445/diff/29367569/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
(right):

https://codereview.adblockplus.org/29361445/diff/29367569/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:323:
private volatile boolean enabled = true;
This field is still mixed with methods code

https://codereview.adblockplus.org/29361445/diff/29367569/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:464:
private List<String> whitelistedDomains;
This field is still mixed with methods code

[anton, 2016-12-16 08:32:52]

https://codereview.adblockplus.org/29361445/diff/29367569/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
(right):

https://codereview.adblockplus.org/29361445/diff/29367569/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:323:
private volatile boolean enabled = true;
On 2016/12/15 14:42:07, diegocarloslima wrote:
> This field is still mixed with methods code

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29367569/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:464:
private List<String> whitelistedDomains;
On 2016/12/15 14:42:07, diegocarloslima wrote:
> This field is still mixed with methods code

Acknowledged.

[diegocarloslima, 2016-12-16 10:12:21]

On 2016/12/16 08:32:52, anton wrote:
>
https://codereview.adblockplus.org/29361445/diff/29367569/libadblockplus-andr...
> File
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
> (right):
> 
>
https://codereview.adblockplus.org/29361445/diff/29367569/libadblockplus-andr...
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:323:
> private volatile boolean enabled = true;
> On 2016/12/15 14:42:07, diegocarloslima wrote:
> > This field is still mixed with methods code
> 
> Acknowledged.
> 
>
https://codereview.adblockplus.org/29361445/diff/29367569/libadblockplus-andr...
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:464:
> private List<String> whitelistedDomains;
> On 2016/12/15 14:42:07, diegocarloslima wrote:
> > This field is still mixed with methods code
> 
> Acknowledged.

LGTM

[anton, 2016-12-16 11:09:49]

https://codereview.adblockplus.org/29361445/diff/29368568/libadblockplus-andr...
File libadblockplus-android/jni/Utils.h (right):

https://codereview.adblockplus.org/29361445/diff/29368568/libadblockplus-andr...
libadblockplus-android/jni/Utils.h:151: jstring JniStdStringToJava(JNIEnv* env,
std::string str);
BTW it can be used in some other files too instead of inlined
'env->NewStringUTF(... .c_str())'

[Felix Dahlke, 2016-12-16 13:40:51]

LGTM!

I only did a very high level review, but it looks pretty good. If you want you
can land this as-is and deal with my comments in follow-up commits.

One thing I still want to go over is the naming of all the publicly available
objects, since that is not so easy to change once this has users. But we can
also do that in a follow-up - time to land this.

https://codereview.adblockplus.org/29361445/diff/29368568/libadblockplus-andr...
File libadblockplus-android-settings/build.xml (right):

https://codereview.adblockplus.org/29361445/diff/29368568/libadblockplus-andr...
libadblockplus-android-settings/build.xml:69: Walkaround for the issue: ant does
not copy assets from libraries by default
Sure you mean a https://en.wikipedia.org/wiki/Walkaround? :D

https://codereview.adblockplus.org/29361445/diff/29368568/libadblockplus-andr...
File libadblockplus-android-webview/assets/hide.js (right):

https://codereview.adblockplus.org/29361445/diff/29368568/libadblockplus-andr...
libadblockplus-android-webview/assets/hide.js:8: //        console.log('testing
selector ' + selector);
Nit: Add {{DEBUG}} or remove?

https://codereview.adblockplus.org/29361445/diff/29368568/libadblockplus-andr...
File
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java
(right):

https://codereview.adblockplus.org/29361445/diff/29368568/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:615:
* WebViewClient for API pre 21
I think we should just strike the <21 use case and require 21 as a min version.
Easier, and we really need the referrer chain / frame structure.

https://codereview.adblockplus.org/29361445/diff/29368568/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:1370:
//     the user can see element visible even though it was hidden on dom event
Would it make sense to create a follow-up issue to look into this some more?

[anton, 2016-12-19 13:22:19]

https://codereview.adblockplus.org/29361445/diff/29368568/libadblockplus-andr...
File libadblockplus-android-settings/build.xml (right):

https://codereview.adblockplus.org/29361445/diff/29368568/libadblockplus-andr...
libadblockplus-android-settings/build.xml:69: Walkaround for the issue: ant does
not copy assets from libraries by default
On 2016/12/16 13:40:45, Felix Dahlke wrote:
> Sure you mean a https://en.wikipedia.org/wiki/Walkaround :D

It's `workaround`, i will fix it

https://codereview.adblockplus.org/29361445/diff/29368568/libadblockplus-andr...
File libadblockplus-android-webview/assets/hide.js (right):

https://codereview.adblockplus.org/29361445/diff/29368568/libadblockplus-andr...
libadblockplus-android-webview/assets/hide.js:8: //        console.log('testing
selector ' + selector);
On 2016/12/16 13:40:45, Felix Dahlke wrote:
> Nit: Add {{DEBUG}} or remove?

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29368568/libadblockplus-andr...
File
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java
(right):

https://codereview.adblockplus.org/29361445/diff/29368568/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:615:
* WebViewClient for API pre 21
On 2016/12/16 13:40:48, Felix Dahlke wrote:
> I think we should just strike the <21 use case and require 21 as a min
version.
> Easier, and we really need the referrer chain / frame structure.

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29368568/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:615:
* WebViewClient for API pre 21
On 2016/12/16 13:40:48, Felix Dahlke wrote:
> I think we should just strike the <21 use case and require 21 as a min
version.
> Easier, and we really need the referrer chain / frame structure.

Acknowledged.

https://codereview.adblockplus.org/29361445/diff/29368568/libadblockplus-andr...
libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java:1370:
//     the user can see element visible even though it was hidden on dom event
On 2016/12/16 13:40:48, Felix Dahlke wrote:
> Would it make sense to create a follow-up issue to look into this some more?

Probably it makes sense. I've found the same behaviour on desktop Chrome as it
tries to show asap and render asap and does not wait for event to be fired.

[diegocarloslima, 2016-12-19 16:14:15]

https://codereview.adblockplus.org/29361445/diff/29369147/README.md
File README.md (right):

https://codereview.adblockplus.org/29361445/diff/29369147/README.md#newcode165
README.md:165: Retain Adblock instance in activity `onCreate` in synchronous
mode (it may take few seconds):
Shouldn't we also change from Adblock to AdblockHelper here?

https://codereview.adblockplus.org/29361445/diff/29369147/README.md#newcode177
README.md:177: Release Adblock instance in activity `onDestroy`:
Shouldn't we also change from Adblock to AdblockHelper here?

[anton, 2016-12-19 19:12:19]

https://codereview.adblockplus.org/29361445/diff/29369147/README.md
File README.md (right):

https://codereview.adblockplus.org/29361445/diff/29369147/README.md#newcode165
README.md:165: Retain Adblock instance in activity `onCreate` in synchronous
mode (it may take few seconds):
On 2016/12/19 16:14:11, diegocarloslima wrote:
> Shouldn't we also change from Adblock to AdblockHelper here?

As for me, no. We retain `AdblockEngine` instance to be more correct. Neither
Adblock nor AdblockHelper.

https://codereview.adblockplus.org/29361445/diff/29369147/README.md#newcode177
README.md:177: Release Adblock instance in activity `onDestroy`:
On 2016/12/19 16:14:11, diegocarloslima wrote:
> Shouldn't we also change from Adblock to AdblockHelper here?

Same here

[Felix Dahlke, 2016-12-19 20:55:18]

Can't see the renamings either. But assuming nothing else changed - LGTM. (I
agree with Anton, the README seems fine as is for now. I think we might want to
improve the instructions some more later anyway.)