Issue 5010 - Allow to preload subscription files

Issue 5010 - Allow to preload subscription files

[anton, 2017-03-20 08:28:46]

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
File libadblockplus-android-tests/build.gradle (right):

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android-tests/build.gradle:21: minSdkVersion 11
It's because of libadblockplus-android min version increased from 9 to 11

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
File libadblockplus-android-webviewapp/res/raw/easylist_min.txt (right):

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android-webviewapp/res/raw/easylist_min.txt:1: [Adblock Plus 2.0]
we need to choose what file should be used

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
File libadblockplus-android-webviewapp/res/raw/keypartner_whitelist.txt (right):

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android-webviewapp/res/raw/keypartner_whitelist.txt:1: [Adblock
Plus 2.0]
we need to choose what file should be used

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

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android/build.gradle:8: minSdkVersion 11
I had to increase from 9 to 11 to use
https://developer.android.com/reference/android/content/SharedPreferences.htm...,
java.util.Set<java.lang.String>)
Edit

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

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:105:
public static class Builder
I had to refactor this monster-ctor to builder as arguments list is growing and
the arguments can be used independently.
For example, now i had to add preloaded subscriptions arguments and it meant i
had to add new ctor with other areguments passed as nulls. However adding new
arguments would add this ctor even worse.

I decided not to use struct-like approach with one new class like
'AdlockEngineParams' with all these arguments as class fields. To be discussed

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:205:
createEngines();
this is completely strange that webRequest should be ready.
The problem is that JsEngine starts using `webRequest` for update immediately
after it's created (ctor) and if it's not yet assigned it fails to update. So we
had to set webrequest property of just created jsengine instance ASAP! It could
be easily fixed by adding webrequest argument to ctor or (better but more
difficult) changing core logics

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:228:
public static Builder builder(AppInfo appInfo, String basePath)
i used the same approach (`builder()`) instead of `new
AdblockEngine.Builder..()` to be consistent with `AppInfo.builder()` though `new
.. .Builder()` is more frequently used in Android. e.g.
https://developer.android.com/reference/android/app/AlertDialog.Builder.html

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:255:
// we need to force update subscriptions ASAP after preloaded one is returned
Please think about it while reviewing!

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequestResourceWrapper.java
(right):

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequestResourceWrapper.java:92:
Map<String, Integer> URLtoResourceIdMap,
using map allows to pass multiple mappings and describe preloaded subscriptions
for multiple locales

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequestResourceWrapper.java:209:
public interface Listener
listener is introduced for the only purpose - we need to force update
subscriptions right after we returned preloaded response.
I decided not to pass FilterEngine instance here directly and not to have this
strong logics, but delegate the required actions to app.
So it's up to app logics if we need to force update or not. BTW listener is
optional

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequestResourceWrapper.java:226:
public static class SharedPrefsStorage implements Storage
The only iml for now. Since we use SharedPrefsStorage for adblock settings too,
i assume we don't need another impls for now.

[anton, 2017-03-20 08:32:23]

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
File libadblockplus-android-webviewapp/res/raw/easylist_min.txt (right):

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android-webviewapp/res/raw/easylist_min.txt:1: [Adblock Plus 2.0]
Also it should be discussed where to store preloaded subscriptions files - in
the the app resources or library resources.
It's undesired to store them in lib resources as it makes it less
general-purpose and all the lib users will include [unmneeded] files

[diegocarloslima, 2017-03-20 15:13:12]

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
File libadblockplus-android-webviewapp/res/raw/easylist_min.txt (right):

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android-webviewapp/res/raw/easylist_min.txt:1: [Adblock Plus 2.0]
On 2017/03/20 08:28:45, anton wrote:
> we need to choose what file should be used

easylist_min is fine for me, but a few other suggestions: easylist_lite,
easylist_reduced, easylist_lightweight. For the storage, I do agree that storing
on the app resources is better

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
File libadblockplus-android-webviewapp/res/raw/keypartner_whitelist.txt (right):

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android-webviewapp/res/raw/keypartner_whitelist.txt:1: [Adblock
Plus 2.0]
On 2017/03/20 08:28:45, anton wrote:
> we need to choose what file should be used

I didn't like this name that much. I would prefer it to be consistent with the
naming of easylist, changing it to whitelist_min

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

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:105:
public static class Builder
On 2017/03/20 08:28:45, anton wrote:
> I had to refactor this monster-ctor to builder as arguments list is growing
and
> the arguments can be used independently.
> For example, now i had to add preloaded subscriptions arguments and it meant i
> had to add new ctor with other areguments passed as nulls. However adding new
> arguments would add this ctor even worse.
> 
> I decided not to use struct-like approach with one new class like
> 'AdlockEngineParams' with all these arguments as class fields. To be discussed

I liked the Builder approach, but as I pointed out on Issue 4948, we need to
either make this issue blocking the Issue 4948 and include here the
isAllowedConnectionCallback param or perform this refactoring in a follow up
issue

[anton, 2017-03-21 05:54:47]

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
File libadblockplus-android-webviewapp/res/raw/easylist_min.txt (right):

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android-webviewapp/res/raw/easylist_min.txt:1: [Adblock Plus 2.0]
On 2017/03/20 15:13:12, diegocarloslima wrote:
> On 2017/03/20 08:28:45, anton wrote:
> > we need to choose what file should be used
> 
> easylist_min is fine for me, but a few other suggestions: easylist_lite,
> easylist_reduced, easylist_lightweight. For the storage, I do agree that
storing
> on the app resources is better

oh, no. i meant that there are multiple stripped files from stephan and we need
to pick one (or multiple but it's undesired because of final file size).
so the question is not about file name in asset but what stripped file to choose
for preloading.

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

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:105:
public static class Builder
On 2017/03/20 15:13:12, diegocarloslima wrote:
> On 2017/03/20 08:28:45, anton wrote:
> > I had to refactor this monster-ctor to builder as arguments list is growing
> and
> > the arguments can be used independently.
> > For example, now i had to add preloaded subscriptions arguments and it meant
i
> > had to add new ctor with other areguments passed as nulls. However adding
new
> > arguments would add this ctor even worse.
> > 
> > I decided not to use struct-like approach with one new class like
> > 'AdlockEngineParams' with all these arguments as class fields. To be
discussed
> 
> I liked the Builder approach, but as I pointed out on Issue 4948, we need to
> either make this issue blocking the Issue 4948 and include here the
> isAllowedConnectionCallback param or perform this refactoring in a follow up
> issue

the issues are related by the code (the same file changed) but not the meaning.
i'm going to merge the changes when merging to main branch. i can prepare new
code review after that code review approved but i'm having feeling that this
code review does not depend on that one directly. is that okay for you?

[diegocarloslima, 2017-03-28 13:47:15]

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
(right):

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:42:
public static final String BASE_PATH_DIRECTORY = "adblock";
I would suggest to move this constant to AdblockEngine. That way, its easier to
keep consistency between the classes that uses this path (for instance,
AdblockHelper and AdblockWebView)

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:59:
private Map<String, Integer> URLtoResourceIdMap;
'toResource' should be 'ToResource'. Also, it's recommended by Android
guidelines to treat abbreviations/acronyms as words: So here it should be 'url'
instead of 'URL'. See
https://source.android.com/source/code-style.html#treat-acronyms-as-words

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:120:
public void preloadSubscriptions(String preferenceName, Map<String, Integer>
URLtoResourceIdMap)
same suggestion here, change 'URLtoResourceIdMap' to 'urlToResourceIdMap'

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
File libadblockplus-android-webviewapp/res/raw/easylist_min.txt (right):

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android-webviewapp/res/raw/easylist_min.txt:1: [Adblock Plus 2.0]
On 2017/03/21 05:54:47, anton wrote:
> On 2017/03/20 15:13:12, diegocarloslima wrote:
> > On 2017/03/20 08:28:45, anton wrote:
> > > we need to choose what file should be used
> > 
> > easylist_min is fine for me, but a few other suggestions: easylist_lite,
> > easylist_reduced, easylist_lightweight. For the storage, I do agree that
> storing
> > on the app resources is better
> 
> oh, no. i meant that there are multiple stripped files from stephan and we
need
> to pick one (or multiple but it's undesired because of final file size).
> so the question is not about file name in asset but what stripped file to
choose
> for preloading.

Got it :) Well, our choice needs do be based in some data/filter usage
statistics to give a decent blocking experience with a considerable reduction in
the number of filter rules.

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

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:105:
public static class Builder
On 2017/03/21 05:54:47, anton wrote:
> On 2017/03/20 15:13:12, diegocarloslima wrote:
> > On 2017/03/20 08:28:45, anton wrote:
> > > I had to refactor this monster-ctor to builder as arguments list is
growing
> > and
> > > the arguments can be used independently.
> > > For example, now i had to add preloaded subscriptions arguments and it
meant
> i
> > > had to add new ctor with other areguments passed as nulls. However adding
> new
> > > arguments would add this ctor even worse.
> > > 
> > > I decided not to use struct-like approach with one new class like
> > > 'AdlockEngineParams' with all these arguments as class fields. To be
> discussed
> > 
> > I liked the Builder approach, but as I pointed out on Issue 4948, we need to
> > either make this issue blocking the Issue 4948 and include here the
> > isAllowedConnectionCallback param or perform this refactoring in a follow up
> > issue
> 
> the issues are related by the code (the same file changed) but not the
meaning.
> i'm going to merge the changes when merging to main branch. i can prepare new
> code review after that code review approved but i'm having feeling that this
> code review does not depend on that one directly. is that okay for you?

You're right, the two issues don't depend on each other by their
meaning/feature-wise. But they do have conflicts on each other, which require a
further merge step. I just feel like that is important to always have a second
pair of eyes looking on sensitive changes to the code. We can ask Felix about
what he thinks is best to do on these cases.

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:255:
// we need to force update subscriptions ASAP after preloaded one is returned
On 2017/03/20 08:28:45, anton wrote:
> Please think about it while reviewing!

Is this onIntercepted() callback always called by the same thread? If so, this
looks okay for me. Otherwise, some thread sync is needed here.

[anton, 2017-03-28 13:59:06]

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
(right):

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:42:
public static final String BASE_PATH_DIRECTORY = "adblock";
On 2017/03/28 13:47:15, diegocarloslima wrote:
> I would suggest to move this constant to AdblockEngine. That way, its easier
to
> keep consistency between the classes that uses this path (for instance,
> AdblockHelper and AdblockWebView)

Acknowledged.

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:59:
private Map<String, Integer> URLtoResourceIdMap;
On 2017/03/28 13:47:15, diegocarloslima wrote:
> 'toResource' should be 'ToResource'. Also, it's recommended by Android
> guidelines to treat abbreviations/acronyms as words: So here it should be
'url'
> instead of 'URL'. See
> https://source.android.com/source/code-style.html#treat-acronyms-as-words

Acknowledged.

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:120:
public void preloadSubscriptions(String preferenceName, Map<String, Integer>
URLtoResourceIdMap)
On 2017/03/28 13:47:15, diegocarloslima wrote:
> same suggestion here, change 'URLtoResourceIdMap' to 'urlToResourceIdMap'

Acknowledged.

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

https://codereview.adblockplus.org/29389555/diff/29389556/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:255:
// we need to force update subscriptions ASAP after preloaded one is returned
On 2017/03/28 13:47:15, diegocarloslima wrote:
> On 2017/03/20 08:28:45, anton wrote:
> > Please think about it while reviewing!
> 
> Is this onIntercepted() callback always called by the same thread? If so, this
> looks okay for me. Otherwise, some thread sync is needed here.

It's invoked in background thread (most likely) and it's up to
core/libadblcokplus to create it. We do sync (using android handler), see
AdblockEngine and it's processed in main thread after it.

[diegocarloslima, 2017-03-30 09:42:07]

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

https://codereview.adblockplus.org/29389555/diff/29396622/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:237:
new AndroidWebRequestResourceWrapper.Listener()
Since this member variable never changes, you should consider declaring it final

https://codereview.adblockplus.org/29389555/diff/29396622/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:241:
private Handler handler = new Handler(Looper.getMainLooper());
Since this member variable never changes, you should consider declaring it final

https://codereview.adblockplus.org/29389555/diff/29396622/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:243:
private Runnable forceUpdateRunnable = new Runnable()
Since this member variable never changes, you should consider declaring it final

[anton, 2017-03-30 10:27:17]

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

https://codereview.adblockplus.org/29389555/diff/29396622/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:237:
new AndroidWebRequestResourceWrapper.Listener()
On 2017/03/30 09:42:07, diegocarloslima wrote:
> Since this member variable never changes, you should consider declaring it
final

Acknowledged.

https://codereview.adblockplus.org/29389555/diff/29396622/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:241:
private Handler handler = new Handler(Looper.getMainLooper());
On 2017/03/30 09:42:07, diegocarloslima wrote:
> Since this member variable never changes, you should consider declaring it
final

Acknowledged.

[diegocarloslima, 2017-03-30 10:31:54]

On 2017/03/30 10:27:17, anton wrote:
>
https://codereview.adblockplus.org/29389555/diff/29396622/libadblockplus-andr...
> File
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
> (right):
> 
>
https://codereview.adblockplus.org/29389555/diff/29396622/libadblockplus-andr...
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:237:
> new AndroidWebRequestResourceWrapper.Listener()
> On 2017/03/30 09:42:07, diegocarloslima wrote:
> > Since this member variable never changes, you should consider declaring it
> final
> 
> Acknowledged.
> 
>
https://codereview.adblockplus.org/29389555/diff/29396622/libadblockplus-andr...
>
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:241:
> private Handler handler = new Handler(Looper.getMainLooper());
> On 2017/03/30 09:42:07, diegocarloslima wrote:
> > Since this member variable never changes, you should consider declaring it
> final
> 
> Acknowledged.

LGTM

[Felix Dahlke, 2017-03-30 14:15:06]

Only had a high level look to not delay things, but LGTM. Would be great if René
can have a look once he's back though.

(I'm assuming we will implement this properly in adblockpluscore/libadblockplus
at some point, so we can get rid of this hack. But for now it'll do.)

https://codereview.adblockplus.org/29389555/diff/29398694/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
(right):

https://codereview.adblockplus.org/29389555/diff/29398694/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:102:
* @param preferenceName Shared Preferences name to store adlock settings
Typo: "adlock"

[sergei, 2017-03-30 22:14:37]

fix in Patch set #6 LGTM.

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

https://codereview.adblockplus.org/29389555/diff/29398800/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:251:
for (Subscription eachSubscription : subscriptions)
Nit: I would remove "each".