Issue 5010 - Allow to preload subscription files

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

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 31 matching lines @@
 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
 {
+  // default base path to store subscription files in android app
+  public static final String BASE_PATH_DIRECTORY = "adblock";
+
   private static final String TAG = Utils.getTag(AdblockEngine.class);
 
   /*
    * The fields below are volatile because:
    *
    * I encountered JNI related bugs/crashes caused by JNI backed Java objects. It seemed that under
    * certain conditions the objects were optimized away which resulted in crashes when trying to
    * release the object, sometimes even on access.
    *
    * The only solution that really worked was to declare the variables holding the references
@@ skipping 33 matching lines @@
         .setVersion(version)
         .setApplicationVersion(sdkVersion)
         .setLocale(locale)
         .setDevelopmentBuild(developmentBuild)
         .build();
   }
 
   /**
    * 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.

   {
     private Context context;
     private Map<String, Integer> URLtoResourceIdMap;
     private AndroidWebRequestResourceWrapper.Storage resourceStorage;
     private AndroidWebRequest androidWebRequest;
     private AppInfo appInfo;
     private String basePath;
 
     private AdblockEngine engine;
 
@@ skipping 79 matching lines @@
       {
         engine.filterEngine.setFilterChangeCallback(engine.filterChangeCallback);
       }
     }
 
     public AdblockEngine build()
     {
       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;
     }
 
     private void createEngines()
     {
       engine.jsEngine = new JsEngine(appInfo);
       engine.jsEngine.setDefaultFileSystem(basePath);
 
       engine.jsEngine.setWebRequest(engine.webRequest);
 
       engine.logSystem = new AndroidLogSystem();
       engine.jsEngine.setLogSystem(engine.logSystem);
 
       engine.filterEngine = new FilterEngine(engine.jsEngine);
     }
   }
 
   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 new Builder(appInfo, basePath);
   }
 
-  private AndroidWebRequestResourceWrapper.Listener resourceWrapperListener =
+  private final 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()
+    private final Handler handler = new Handler(Looper.getMainLooper());
+
+    private final 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);
+          List<Subscription> subscriptions = filterEngine.getListedSubscriptions();
+          for (Subscription eachSubscription : subscriptions)

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

Nit: I would remove "each".

+          {
+            try
+            {
+              eachSubscription.updateFilters();
+            }
+            finally
+            {
+              eachSubscription.dispose();
+            }
+          }
         }
       }
     };
 
     @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);
     }
   };
@@ skipping 309 matching lines @@
   public void setWhitelistedDomains(List<String> domains)
   {
     this.whitelistedDomains = domains;
   }
 
   public List<String> getWhitelistedDomains()
   {
     return whitelistedDomains;
   }
 }