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

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

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 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
   {
     System.loadLibrary("adblockplus-jni");
     registerNatives();
   }
 
   public static enum ContentType
   {
     OTHER, SCRIPT, IMAGE, STYLESHEET, OBJECT, SUBDOCUMENT, DOCUMENT, XMLHTTPREQUEST,
     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()
   {
     return isFirstRun(this.ptr);
   }
 
   public Filter getFilter(final String text)
   {
     return getFilter(this.ptr, text);
@@ skipping 137 matching lines @@
   /**
    * Schedules updating of a subscription corresponding to the passed URL.
    * @param subscriptionUrl may contain query parameters, only the beginning of the string is used
    *                       to find a corresponding subscription.
    */
   public void updateFiltersAsync(String subscriptionUrl)
   {
     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);
 
   private final static native List<Filter> getListedFilters(long ptr);
 
   private final static native Subscription getSubscription(long ptr, String url);
 
   private final static native List<Subscription> getListedSubscriptions(long ptr);
 
   private final static native List<Subscription> fetchAvailableSubscriptions(long ptr);
@@ skipping 34 matching lines @@
 
   private final static native String getAllowedConnectionType(long ptr);
 
   private final static native void setAcceptableAdsEnabled(long ptr, boolean enabled);
 
   private final static native boolean isAcceptableAdsEnabled(long ptr);
 
   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);
 }