Issue 5010 - Allow to preload subscription files

libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java

Index: libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
diff --git a/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java b/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
index 2b6138245dddd39e780ee5c851f7b584956c9681..757b261971831823a18d9d0b45bfe060b367714e 100644
--- a/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
+++ b/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java
@@ -23,6 +23,7 @@ import java.util.Collection;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
+import java.util.Map;
 import java.util.Set;
 
 import org.adblockplus.libadblockplus.AppInfo;
@@ -36,11 +37,14 @@ import org.adblockplus.libadblockplus.ShowNotificationCallback;
 import org.adblockplus.libadblockplus.Subscription;
 import org.adblockplus.libadblockplus.UpdateAvailableCallback;
 import org.adblockplus.libadblockplus.UpdateCheckDoneCallback;
+import org.adblockplus.libadblockplus.WebRequest;
 
 import android.content.Context;
 import android.content.pm.PackageInfo;
 import android.content.pm.PackageManager.NameNotFoundException;
 import android.os.Build.VERSION;
+import android.os.Handler;
+import android.os.Looper;
 import android.util.Log;
 
 public final class AdblockEngine
@@ -61,19 +65,14 @@ public final class AdblockEngine
   private volatile JsEngine jsEngine;
   private volatile FilterEngine filterEngine;
   private volatile LogSystem logSystem;
-  private volatile AndroidWebRequest webRequest;
+  private volatile WebRequest webRequest;
   private volatile UpdateAvailableCallback updateAvailableCallback;
   private volatile UpdateCheckDoneCallback updateCheckDoneCallback;
   private volatile FilterChangeCallback filterChangeCallback;
   private volatile ShowNotificationCallback showNotificationCallback;
-  private final boolean elemhideEnabled;
+  private volatile boolean elemhideEnabled;
   private volatile boolean enabled = true;
-  private List<String> whitelistedDomains;
-
-  private AdblockEngine(final boolean enableElemhide)
-  {
-    this.elemhideEnabled = enableElemhide;
-  }
+  private volatile List<String> whitelistedDomains;
 
   public static AppInfo generateAppInfo(final Context context, boolean developmentBuild)
   {
@@ -100,59 +99,171 @@ public final class AdblockEngine
         .build();
   }
 
-  public static AdblockEngine create(final AppInfo appInfo,
-                                     final String basePath, boolean enableElemhide,
-                                     UpdateAvailableCallback updateAvailableCallback,
-                                     UpdateCheckDoneCallback updateCheckDoneCallback,
-                                     ShowNotificationCallback showNotificationCallback,
-                                     FilterChangeCallback filterChangeCallback)
+  /**
+   * Builds Adblock engine
+   */
+  public static class Builder

[anton, 2017/03/20 08:28:45]

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

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

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]

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]

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.

   {
-    Log.w(TAG, "Create");
+    private Context context;
+    private Map<String, Integer> URLtoResourceIdMap;
+    private AndroidWebRequestResourceWrapper.Storage resourceStorage;
+    private AndroidWebRequest androidWebRequest;
+    private AppInfo appInfo;
+    private String basePath;
 
-    final AdblockEngine engine = new AdblockEngine(enableElemhide);
+    private AdblockEngine engine;
 
-    engine.jsEngine = new JsEngine(appInfo);
-    engine.jsEngine.setDefaultFileSystem(basePath);
+    protected Builder(final AppInfo appInfo, final String basePath)
+    {
+      engine = new AdblockEngine();
+      engine.elemhideEnabled = true;
+
+      // we can't create JsEngine and FilterEngine right now as it starts to download subscriptions
+      // and requests (AndroidWebRequest and probbaly wrappers) are not specified yet
+      this.appInfo = appInfo;
+      this.basePath = basePath;
+    }
+
+    public Builder enableElementHiding(boolean enable)
+    {
+      engine.elemhideEnabled = enable;
+      return this;
+    }
+
+    public Builder preloadSubscriptions(Context context,
+                                        Map<String, Integer> URLtoResourceIdMap,
+                                        AndroidWebRequestResourceWrapper.Storage storage)
+    {
+      this.context = context;
+      this.URLtoResourceIdMap = URLtoResourceIdMap;
+      this.resourceStorage = storage;
+      return this;
+    }
 
-    engine.logSystem = new AndroidLogSystem();
-    engine.jsEngine.setLogSystem(engine.logSystem);
+    public Builder setUpdateAvailableCallback(UpdateAvailableCallback callback)
+    {
+      engine.updateAvailableCallback = callback;
+      return this;
+    }
 
-    engine.webRequest = new AndroidWebRequest(enableElemhide);
-    engine.jsEngine.setWebRequest(engine.webRequest);
+    public Builder setUpdateCheckDoneCallback(UpdateCheckDoneCallback callback)
+    {
+      engine.updateCheckDoneCallback = callback;
+      return this;
+    }
 
-    engine.filterEngine = new FilterEngine(engine.jsEngine);
+    public Builder setShowNotificationCallback(ShowNotificationCallback callback)
+    {
+      engine.showNotificationCallback = callback;
+      return this;
+    }
 
-    engine.updateAvailableCallback = updateAvailableCallback;
-    if (engine.updateAvailableCallback != null)
+    public Builder setFilterChangeCallback(FilterChangeCallback callback)
     {
-      engine.filterEngine.setUpdateAvailableCallback(updateAvailableCallback);
+      engine.filterChangeCallback = callback;
+      return this;
     }
 
-    engine.updateCheckDoneCallback = updateCheckDoneCallback;
+    private void initRequests()
+    {
+      androidWebRequest = new AndroidWebRequest(engine.elemhideEnabled);
+      engine.webRequest = androidWebRequest;
 
-    engine.showNotificationCallback = showNotificationCallback;
-    if (engine.showNotificationCallback != null)
+      if (URLtoResourceIdMap != null)
+      {
+        AndroidWebRequestResourceWrapper wrapper = new AndroidWebRequestResourceWrapper(
+          context, engine.webRequest, URLtoResourceIdMap, resourceStorage);
+        wrapper.setListener(engine.resourceWrapperListener);
+
+        engine.webRequest = wrapper;
+      }
+    }
+
+    private void initCallbacks()
     {
-      engine.filterEngine.setShowNotificationCallback(showNotificationCallback);
+      if (engine.updateAvailableCallback != null)
+      {
+        engine.filterEngine.setUpdateAvailableCallback(engine.updateAvailableCallback);
+      }
+
+      if (engine.showNotificationCallback != null)
+      {
+        engine.filterEngine.setShowNotificationCallback(engine.showNotificationCallback);
+      }
+
+      if (engine.filterChangeCallback != null)
+      {
+        engine.filterEngine.setFilterChangeCallback(engine.filterChangeCallback);
+      }
     }
 
-    engine.filterChangeCallback = filterChangeCallback;
-    if (engine.filterChangeCallback != null)
+    public AdblockEngine build()
     {
-      engine.filterEngine.setFilterChangeCallback(filterChangeCallback);
+      initRequests();
+
+      // webRequest should be ready to be used passed right after JsEngine is created
+      createEngines();

[anton, 2017/03/20 08:28:45]

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

+
+      initCallbacks();
+
+      androidWebRequest.updateSubscriptionURLs(engine.filterEngine);
+
+      return engine;
     }
 
-    engine.webRequest.updateSubscriptionURLs(engine.filterEngine);
+    private void createEngines()
+    {
+      engine.jsEngine = new JsEngine(appInfo);
+      engine.jsEngine.setDefaultFileSystem(basePath);
+
+      engine.jsEngine.setWebRequest(engine.webRequest);
 
-    return engine;
+      engine.logSystem = new AndroidLogSystem();
+      engine.jsEngine.setLogSystem(engine.logSystem);
+
+      engine.filterEngine = new FilterEngine(engine.jsEngine);
+    }
   }
 
-  public static AdblockEngine create(final AppInfo appInfo,
-                                     final String basePath, boolean elemhideEnabled)
+  public static Builder builder(AppInfo appInfo, String basePath)

[anton, 2017/03/20 08:28:45]

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

   {
-    return create(appInfo, basePath, elemhideEnabled, null, null, null, null);
+    return new Builder(appInfo, basePath);
   }
 
+  private AndroidWebRequestResourceWrapper.Listener resourceWrapperListener =
+    new AndroidWebRequestResourceWrapper.Listener()
+  {
+    private static final int UPDATE_DELAY_MS = 1 * 1000;
+
+    private Handler handler = new Handler(Looper.getMainLooper());
+
+    private Runnable forceUpdateRunnable = new Runnable()
+    {
+      public void run() {
+        // Filter Engine can be already disposed
+        if (filterEngine != null)
+        {
+          Log.d(TAG, "Force update subscriptions");
+          filterEngine.forceUpdateCheck(updateCheckDoneCallback);
+        }
+      }
+    };
+
+    @Override
+    public void onIntercepted(String url, int resourceId)
+    {
+      // we need to force update subscriptions ASAP after preloaded one is returned

[anton, 2017/03/20 08:28:45]

Please think about it while reviewing!

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

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]

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.

+      // but we should note that multiple interceptions (for main easylist and AA) and force update once only
+
+      // adding into main thread queue to avoid concurrency issues (start update while updating)
+      // as usually onIntercepted() is invoked in background thread
+      handler.removeCallbacks(forceUpdateRunnable);
+      handler.postDelayed(forceUpdateRunnable, UPDATE_DELAY_MS);
+
+      Log.d(TAG, "Scheduled force update in " + UPDATE_DELAY_MS);
+    }
+  };
+
   public void dispose()
   {
     Log.w(TAG, "Dispose");