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

libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.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;
+
+public class Platform implements Disposable
+{
+  private final Disposer disposer;
+  protected final long ptr;
+
+  static
+  {
+    System.loadLibrary("adblockplus-jni");
+    registerNatives();
+  }
+
+  public Platform(final LogSystem logSystem, final WebRequest webRequest, final String basePath)
+  {
+    this(ctor(logSystem, webRequest, basePath));
+  }
+
+  public Platform(final WebRequest webRequest, final String basePath)
+  {
+    this(null, webRequest, basePath);
+  }
+
+  public Platform()
+  {
+    this(null, null, null);

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

it would be great to have some comment, how it works without anything passed
(like default paths and behaviour)

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

I have added the comment to the constructor above and removed unused
constructors.

+  }
+
+  protected Platform(final long ptr)
+  {
+    this.ptr = ptr;
+    this.disposer = new Disposer(this, new DisposeWrapper(ptr));
+  }
+
+  public void setUpJsEngine(final AppInfo appInfo)
+  {
+    setUpJsEngine(this.ptr, appInfo);
+  }
+
+  public JsEngine getJsEngine()
+  {
+    return new JsEngine(getJsEnginePtr(this.ptr));

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

why JsEngine accepts js engine ptr, but filter engine accepts platform ptr?
Can both of them accept platform ptr or js/filter engine to be consistent?

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

what's the purpose of creating new instance every time it's requested? why not
create it once in `setUpJsEngine` and save as private property? 

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

Short story is to keep it simple.

The main thing here and for FilterEngine is that I tried to stay closer to C++
interface where Platform returns references to JsEngine and to FilterEngine.
Another thing is that, although it's a tiny thing, I guess, we should not pay
for what we don't use, so if we don't use Java JsEngine (it's used only in
tests) we should not fiddle around with it. Let's say one constructs Platform
and calls getFilterEngine, in this case the member Platform.jsEngine would be
null which looks confusing. One could of course initialize it anyway when it's
created but there are actually several ways to cause a creation of JsEngine and
I would assume that the interface of AdblockPlus::Platform is not stable yet, so
I would propose to wait with such additional functionality in
libadblockplus-android. The idea is maybe not bad, so if there are some
objections then I would propose to make it but under another issue. What is your
opinion?

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

I think that generally corresponding Java classes should use corresponding
pointers to C++ classes, so Java JsEngine is using a pointer to C++ JsEngine and
Java Platform is using a pointer to JniPlatform (I think it counts as a
corresponding C++ class). Basically, the latter (JniPlatform) is introduced
merely in order to support some functionality of Java FilterEngine because a
pointer to C++ FilterEngine is not enough there. I think that for the
consistency it would make sense to have JniFilterEngine with a scheduler and a
pointer to FilterEngine as members. However taking into account how much things
it introduces in addition, the fact that C++ FilterEngine is managed only by C++
Platform and having each additional pointer to it does not look good, and that
Java FilterEngine will be disposable but Java JsEngine not, I would say that
it's an over-engineering. Therefore to keep things simple JniPlatform is used
instead of JniFilterEgnine. Do you think it makes sense to add JniFilterEngine?

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

I'd say `getter` task is to `get`, not `create`. Otherwise i'd prefer it to be
renamed to 'create..' or 'build..'.
 I'd still prefer both engines to be created in `setUp..` methods and just
returned in getters.

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

getXXX methods create the corresponding entities only once and only if they are
not created yet, so they are rather getters. What concerns creation of Java
classes I think it's fine because they are merely tiny wrappers, similar to
std::shared_ptr, for example, so they cannot be counted as the creation of the
entities. BTW, setUp methods are optional and IMO it's a bad design when one has
to call methods in a specific order, until it's in the aim of a class, so I
would like to avoid asking firstly call some `create` method and only then one
may call a getter, in the latter case I would rather remove getters.
I would say that the only "stable" method here is setUpJsEngine, I think it will
stay unchanged, others are not too important right now.
getJsEngine - it exists only to allow users to get access to internals, e.g. in
tests, or where we are not ready yet. Normally it should not be used.
setUpFilterEngine - should be actually called `createFilterEngineAsync` as it's
called in libadblockplus, but I made it simpler here for present.
getFilterEngine - here and in libadblockplus it exists only to simplify the
starting to work with libadblockplus, namely one should not care about
synchronization with asynchronous creation of FilterEngine if all interfaces
have a default implementation. I would like to remove this method, at least in
libadblockplus-android, but it will be possible only after switching to
`createFilterEngineAsync` everywhere which is not a small change.

+  }
+
+  public void setUpFilterEngine(final IsAllowedConnectionCallback isSubscriptionDownloadAllowedCallback)
+  {
+    setUpFilterEngine(this.ptr, isSubscriptionDownloadAllowedCallback);
+  }
+
+  public FilterEngine getFilterEngine()
+  {
+    ensureFilterEngine(this.ptr);

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

what does `ensure..` do? it's absolutely unclear without jumping in c++ code.
I believe some comment is required here

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

Done.
BTW, I expect that the interface of Platform will be changed soon, so if there
are some thoughts it would be good to accumulate them in some issue, what can
also help to make the interface of Platform in libadblockplus better.

+    return new FilterEngine(this.ptr);

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

same here - what's the purpose of creating new instance every time?

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

see comment for getJsEngine

+  }
+
+  @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(LogSystem logSystem, WebRequest webRequest, String basePath);
+
+  private final static native void setUpJsEngine(long ptr, AppInfo appInfo);
+
+  private final static native long getJsEnginePtr(long ptr);
+
+  private final static native void setUpFilterEngine(long ptr, IsAllowedConnectionCallback isSubscriptionDownloadAllowedCallback);
+
+  private final static native void ensureFilterEngine(long ptr);
+
+  private final static native void dtor(long ptr);
+}