Issue 5556 - Update to use libadblockplus revision hg:566f64c8a2a8

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

Index: libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java
diff --git a/libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java b/libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java
index f3d72d8fcd5fb844e30ee7c6b77be9986f149884..4b50519825be40471c2064127a50eb4f146a3dee 100644
--- a/libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java
+++ b/libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java
@@ -19,9 +19,8 @@ package org.adblockplus.libadblockplus;
 
 import java.util.List;
 
-public final class FilterEngine implements Disposable
+public final class FilterEngine
 {
-  private final Disposer disposer;
   protected final long ptr;

[anton, 2017/09/06 06:21:27]

since now it's platform ptr, not filter engine ptr, i'd suggest to rename it to
`platformPtr` and rename method arguments too

 
   static
@@ -36,15 +35,9 @@ public final class FilterEngine implements Disposable
     OBJECT_SUBREQUEST, FONT, MEDIA
   }
 
-  public FilterEngine(final JsEngine jsEngine, final IsAllowedConnectionCallback isSubscriptionDownloadAllowedCallback)
+  FilterEngine(/* JniPlatform */long ptr)

[anton, 2017/09/06 06:21:27]

if it's renamed to `platformPtr` there is no need in comment

[anton, 2017/09/06 06:21:27]

`public` here is required. Otherwise it's `package` so it's working for the
classes from the same package only.

[sergei, 2017/09/08 09:45:01]

Totally agree with the constructor argument, renamed here, but I'm not sure
about the member `ptr`. See my comment in JniFilterEngine.cpp, it plays the role
of an opaque native pointer and I think it's not necessary and maybe we even
should not expose its nature here.

[sergei, 2017/09/08 09:45:01]

I actually wanted it, so FilterEngine can be created only by a class from this
package, namely only by Platform.

[anton, 2017/09/08 10:19:19]

Let's make it `public`.
We did not introduce `package` scope in files though we could do it as the point
is absolutely the same and i don't see any sense to do it in 1 particular case.

[sergei, 2017/09/08 12:25:30]

But in this case anyone can call a constructor of FilterEngine, it definitely
should not be public, it should be private but in Java there is no concept of
friend classes, so the choice is between protected and no modifier which is
package-private. Protected makes a little sense because it's rather for
inheritance but the class if final, so package private describes the intention
better than other modifiers.

[diegocarloslima, 2017/09/08 20:42:41]

Since this project is meant to be distributed as a library, it is really
important to have the appropriate access modifier for each class member. If this
class isn't meant to be constructed by 3rd party classes, I agree with Sergei
that we should use the default modifier. If we have other places where is
intended to be only visible by our package, I think we should also change it.
That helps to avoid misuse of our library and might reduce the time spent giving
support for 3rd party implementation

   {
-    this.ptr = ctor(jsEngine.ptr, isSubscriptionDownloadAllowedCallback);
-    this.disposer = new Disposer(this, new DisposeWrapper(this.ptr));
-  }
-
-  public FilterEngine(final JsEngine jsEngine)
-  {
-    this(jsEngine, null);
+    this.ptr = ptr;
   }
 
   public boolean isFirstRun()
@@ -202,32 +195,8 @@ public final class FilterEngine implements Disposable
     updateFiltersAsync(this.ptr, subscriptionUrl);
   }
 
-  @Override
-  public void dispose()
-  {
-    this.disposer.dispose();
-  }
-
-  private final static class DisposeWrapper implements Disposable
-  {
-    private final long ptr;
-
-    public DisposeWrapper(final long ptr)
-    {
-      this.ptr = ptr;
-    }
-
-    @Override
-    public void dispose()
-    {
-      dtor(this.ptr);
-    }
-  }
-
   private final static native void registerNatives();
 
-  private final static native long ctor(long jsEnginePtr, IsAllowedConnectionCallback isSubscriptionDownloadAllowedCallback);
-
   private final static native boolean isFirstRun(long ptr);

[anton, 2017/09/06 06:21:27]

i'd suggest to rename all `ptr` to `platformPtr` in all methods (here and below)

[sergei, 2017/09/08 09:45:01]

see my thoughts about opaque pointer role, let's discuss it in
JniFilterEngine.cpp

 
   private final static native Filter getFilter(long ptr, String text);
@@ -283,6 +252,4 @@ public final class FilterEngine implements Disposable
   private final static native String getAcceptableAdsSubscriptionURL(long ptr);
 
   private final static native void updateFiltersAsync(long ptr, String subscriptionUrl);
-
-  private final static native void dtor(long ptr);
 }