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

add Platform class because new libadblockplus has it, namely AdblockPlus::Platform, which is basically an entry point and it's responsible for JsEngine, FilterEngine and platform dependent functionality, e.g. File System. Therefore Java JsEngine and FilterEngine are not disposable anymore because the corresponding instances of C++ classes are destroyed when Platform is destroyed.

# based on https://codereview.adblockplus.org/29536604/
COLLABORATOR=anton@adblockplus.org

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

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
File libadblockplus-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:51: AdblockPlus::FilterEngine&
GetFilterEngineRef(jlong ptr)
rename to `platformPtr`

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:57: static jboolean JNICALL
JniIsFirstRun(JNIEnv* env, jclass clazz, jlong ptr)
i'd suggest to rename all `ptr` to `platformPtr` in all methods

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java
(right):

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:24:
protected final long ptr;
since now it's platform ptr, not filter engine ptr, i'd suggest to rename it to
`platformPtr` and rename method arguments too

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:38:
FilterEngine(/* JniPlatform */long ptr)
if it's renamed to `platformPtr` there is no need in comment

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:38:
FilterEngine(/* JniPlatform */long ptr)
`public` here is required. Otherwise it's `package` so it's working for the
classes from the same package only.

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:200:
private final static native boolean isFirstRun(long ptr);
i'd suggest to rename all `ptr` to `platformPtr` in all methods (here and below)

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java
(right):

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java:32:
JsEngine(final long ptr)
`public` here is required (unless it's done intentionally)

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java
(right):

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:43:
this(null, null, null);
it would be great to have some comment, how it works without anything passed
(like default paths and behaviour)

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:59:
return new JsEngine(getJsEnginePtr(this.ptr));
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?

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:59:
return new JsEngine(getJsEnginePtr(this.ptr));
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?

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:69:
ensureFilterEngine(this.ptr);
what does `ensure..` do? it's absolutely unclear without jumping in c++ code.
I believe some comment is required here

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:70:
return new FilterEngine(this.ptr);
same here - what's the purpose of creating new instance every time?

[sergei, 2017-09-08 09:45:03]

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
File libadblockplus-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:51: AdblockPlus::FilterEngine&
GetFilterEngineRef(jlong ptr)
On 2017/09/06 06:21:27, anton wrote:
> rename to `platformPtr`

renamed to jniPlatformPtr

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:57: static jboolean JNICALL
JniIsFirstRun(JNIEnv* env, jclass clazz, jlong ptr)
On 2017/09/06 06:21:27, anton wrote:
> i'd suggest to rename all `ptr` to `platformPtr` in all methods

I'm not sure that there is a real necessity in it because here `ptr` plays a
role of an opaque pointer and since we are not working with it directly only
through GetFilterEngineRef I would propose to leave `ptr` argument in these
methods, but rename it only where its nature is actually used, namely in
GetFilterEngineRef and in JniUpdateFiltersAsync. What do you think about it?
BTW, just below we even use generic `engine` however we keep in mind the actual
kind of the engine.

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java
(right):

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:38:
FilterEngine(/* JniPlatform */long ptr)
On 2017/09/06 06:21:27, anton wrote:
> if it's renamed to `platformPtr` there is no need in comment

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.

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:38:
FilterEngine(/* JniPlatform */long ptr)
On 2017/09/06 06:21:27, anton wrote:
> `public` here is required. Otherwise it's `package` so it's working for the
> classes from the same package only.

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

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:200:
private final static native boolean isFirstRun(long ptr);
On 2017/09/06 06:21:27, anton wrote:
> i'd suggest to rename all `ptr` to `platformPtr` in all methods (here and
below)

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

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java
(right):

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java:32:
JsEngine(final long ptr)
On 2017/09/06 06:21:27, anton wrote:
> `public` here is required (unless it's done intentionally)

It's done indeed deliberately in order to express that an arbitrary
instantiation of JsEngine is not intended, only Platform can and may do it.

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java
(right):

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:43:
this(null, null, null);
On 2017/09/06 06:21:28, anton wrote:
> it would be great to have some comment, how it works without anything passed
> (like default paths and behaviour)

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

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:59:
return new JsEngine(getJsEnginePtr(this.ptr));
On 2017/09/06 06:21:27, anton wrote:
> 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?

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?

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:59:
return new JsEngine(getJsEnginePtr(this.ptr));
On 2017/09/06 06:21:28, anton wrote:
> 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? 

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?

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:69:
ensureFilterEngine(this.ptr);
On 2017/09/06 06:21:28, anton wrote:
> what does `ensure..` do? it's absolutely unclear without jumping in c++ code.
> I believe some comment is required here

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.

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:70:
return new FilterEngine(this.ptr);
On 2017/09/06 06:21:28, anton wrote:
> same here - what's the purpose of creating new instance every time?

see comment for getJsEngine

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

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
File libadblockplus-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:57: static jboolean JNICALL
JniIsFirstRun(JNIEnv* env, jclass clazz, jlong ptr)
On 2017/09/08 09:45:01, sergei wrote:
> On 2017/09/06 06:21:27, anton wrote:
> > i'd suggest to rename all `ptr` to `platformPtr` in all methods
> 
> I'm not sure that there is a real necessity in it because here `ptr` plays a
> role of an opaque pointer and since we are not working with it directly only
> through GetFilterEngineRef I would propose to leave `ptr` argument in these
> methods, but rename it only where its nature is actually used, namely in
> GetFilterEngineRef and in JniUpdateFiltersAsync. What do you think about it?
> BTW, just below we even use generic `engine` however we keep in mind the
actual
> kind of the engine.

i'd prefer it to be renamed as previously it was c++ filter engine ref actually
and now it's platform actually (JniPlatform instance to be more detailed). This
could introduce misunderstanding.

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java
(right):

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:38:
FilterEngine(/* JniPlatform */long ptr)
On 2017/09/08 09:45:01, sergei wrote:
> On 2017/09/06 06:21:27, anton wrote:
> > `public` here is required. Otherwise it's `package` so it's working for the
> > classes from the same package only.
> 
> I actually wanted it, so FilterEngine can be created only by a class from this
> package, namely only by Platform.

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.

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java
(right):

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:59:
return new JsEngine(getJsEnginePtr(this.ptr));
On 2017/09/08 09:45:02, sergei wrote:
> On 2017/09/06 06:21:28, anton wrote:
> > 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? 
> 
> 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?

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.

[anton, 2017-09-08 10:54:23]

https://codereview.adblockplus.org/29536629/diff/29539610/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java
(right):

https://codereview.adblockplus.org/29536629/diff/29539610/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:31: //
If an interface parameter value is null then a default implementation is
it would be better to have javadoc (/**, not just comment) here

[sergei, 2017-09-08 12:25:31]

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
File libadblockplus-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:57: static jboolean JNICALL
JniIsFirstRun(JNIEnv* env, jclass clazz, jlong ptr)
On 2017/09/08 10:19:18, anton wrote:
> On 2017/09/08 09:45:01, sergei wrote:
> > On 2017/09/06 06:21:27, anton wrote:
> > > i'd suggest to rename all `ptr` to `platformPtr` in all methods
> > 
> > I'm not sure that there is a real necessity in it because here `ptr` plays a
> > role of an opaque pointer and since we are not working with it directly only
> > through GetFilterEngineRef I would propose to leave `ptr` argument in these
> > methods, but rename it only where its nature is actually used, namely in
> > GetFilterEngineRef and in JniUpdateFiltersAsync. What do you think about it?
> > BTW, just below we even use generic `engine` however we keep in mind the
> actual
> > kind of the engine.
> 
> i'd prefer it to be renamed as previously it was c++ filter engine ref
actually
> and now it's platform actually (JniPlatform instance to be more detailed).
This
> could introduce misunderstanding.

I would like to wait for the third opinion here. I think that the only meaning
`ptr` is  carrying here is a native pointer held by Java class and it should not
show in each method what is actually behind it. Why should it generate a lot of
changes if we change the nature of this pointer if it's used as an opaque
integer value?

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java
(right):

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:38:
FilterEngine(/* JniPlatform */long ptr)
On 2017/09/08 10:19:19, anton wrote:
> On 2017/09/08 09:45:01, sergei wrote:
> > On 2017/09/06 06:21:27, anton wrote:
> > > `public` here is required. Otherwise it's `package` so it's working for
the
> > > classes from the same package only.
> > 
> > I actually wanted it, so FilterEngine can be created only by a class from
this
> > package, namely only by Platform.
> 
> 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.

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.

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java
(right):

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:59:
return new JsEngine(getJsEnginePtr(this.ptr));
On 2017/09/08 10:19:19, anton wrote:
> On 2017/09/08 09:45:02, sergei wrote:
> > On 2017/09/06 06:21:28, anton wrote:
> > > 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? 
> > 
> > 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?
> 
> 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.
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.

https://codereview.adblockplus.org/29536629/diff/29539610/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java
(right):

https://codereview.adblockplus.org/29536629/diff/29539610/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:31: //
If an interface parameter value is null then a default implementation is
On 2017/09/08 10:54:22, anton wrote:
> it would be better to have javadoc (/**, not just comment) here

Done.

[diegocarloslima, 2017-09-08 20:42:42]

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
File libadblockplus-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:57: static jboolean JNICALL
JniIsFirstRun(JNIEnv* env, jclass clazz, jlong ptr)
On 2017/09/08 12:25:30, sergei wrote:
> On 2017/09/08 10:19:18, anton wrote:
> > On 2017/09/08 09:45:01, sergei wrote:
> > > On 2017/09/06 06:21:27, anton wrote:
> > > > i'd suggest to rename all `ptr` to `platformPtr` in all methods
> > > 
> > > I'm not sure that there is a real necessity in it because here `ptr` plays
a
> > > role of an opaque pointer and since we are not working with it directly
only
> > > through GetFilterEngineRef I would propose to leave `ptr` argument in
these
> > > methods, but rename it only where its nature is actually used, namely in
> > > GetFilterEngineRef and in JniUpdateFiltersAsync. What do you think about
it?
> > > BTW, just below we even use generic `engine` however we keep in mind the
> > actual
> > > kind of the engine.
> > 
> > i'd prefer it to be renamed as previously it was c++ filter engine ref
> actually
> > and now it's platform actually (JniPlatform instance to be more detailed).
> This
> > could introduce misunderstanding.
> 
> I would like to wait for the third opinion here. I think that the only meaning
> `ptr` is  carrying here is a native pointer held by Java class and it should
not
> show in each method what is actually behind it. Why should it generate a lot
of
> changes if we change the nature of this pointer if it's used as an opaque
> integer value?

As I could see, the only place where the ptr had a specific name was in the
JniCtor. Also, for every other function, the ptr type is implicit. I would
suggest leaving 'ptr' in this code review, and if you really think that this may
cause readability problems, we can change it everywhere (in order to be
consistent) in a follow up issue (or even add some comments/documentation).

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java
(right):

https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:38:
FilterEngine(/* JniPlatform */long ptr)
On 2017/09/08 12:25:30, sergei wrote:
> On 2017/09/08 10:19:19, anton wrote:
> > On 2017/09/08 09:45:01, sergei wrote:
> > > On 2017/09/06 06:21:27, anton wrote:
> > > > `public` here is required. Otherwise it's `package` so it's working for
> the
> > > > classes from the same package only.
> > > 
> > > I actually wanted it, so FilterEngine can be created only by a class from
> this
> > > package, namely only by Platform.
> > 
> > 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.
> 
> 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.

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

[anton, 2017-09-11 09:24:00]

On 2017/09/08 12:25:31, sergei wrote:
>
https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
> File libadblockplus-android/jni/JniFilterEngine.cpp (right):
> 
>
https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
> libadblockplus-android/jni/JniFilterEngine.cpp:57: static jboolean JNICALL
> JniIsFirstRun(JNIEnv* env, jclass clazz, jlong ptr)
> On 2017/09/08 10:19:18, anton wrote:
> > On 2017/09/08 09:45:01, sergei wrote:
> > > On 2017/09/06 06:21:27, anton wrote:
> > > > i'd suggest to rename all `ptr` to `platformPtr` in all methods
> > > 
> > > I'm not sure that there is a real necessity in it because here `ptr` plays
a
> > > role of an opaque pointer and since we are not working with it directly
only
> > > through GetFilterEngineRef I would propose to leave `ptr` argument in
these
> > > methods, but rename it only where its nature is actually used, namely in
> > > GetFilterEngineRef and in JniUpdateFiltersAsync. What do you think about
it?
> > > BTW, just below we even use generic `engine` however we keep in mind the
> > actual
> > > kind of the engine.
> > 
> > i'd prefer it to be renamed as previously it was c++ filter engine ref
> actually
> > and now it's platform actually (JniPlatform instance to be more detailed).
> This
> > could introduce misunderstanding.
> 
> I would like to wait for the third opinion here. I think that the only meaning
> `ptr` is  carrying here is a native pointer held by Java class and it should
not
> show in each method what is actually behind it. Why should it generate a lot
of
> changes if we change the nature of this pointer if it's used as an opaque
> integer value?
> 
>
https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
> File
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java
> (right):
> 
>
https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
>
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:38:
> FilterEngine(/* JniPlatform */long ptr)
> On 2017/09/08 10:19:19, anton wrote:
> > On 2017/09/08 09:45:01, sergei wrote:
> > > On 2017/09/06 06:21:27, anton wrote:
> > > > `public` here is required. Otherwise it's `package` so it's working for
> the
> > > > classes from the same package only.
> > > 
> > > I actually wanted it, so FilterEngine can be created only by a class from
> this
> > > package, namely only by Platform.
> > 
> > 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.
> 
> 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.
> 
>
https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
> File libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java
> (right):
> 
>
https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
> libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:59:
> return new JsEngine(getJsEnginePtr(this.ptr));
> On 2017/09/08 10:19:19, anton wrote:
> > On 2017/09/08 09:45:02, sergei wrote:
> > > On 2017/09/06 06:21:28, anton wrote:
> > > > 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? 
> > > 
> > > 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?
> > 
> > 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.
> 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.
> 
>
https://codereview.adblockplus.org/29536629/diff/29539610/libadblockplus-andr...
> File libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java
> (right):
> 
>
https://codereview.adblockplus.org/29536629/diff/29539610/libadblockplus-andr...
> libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:31: //
> If an interface parameter value is null then a default implementation is
> On 2017/09/08 10:54:22, anton wrote:
> > it would be better to have javadoc (/**, not just comment) here
> 
> Done.

LGTM

[diegocarloslima, 2017-09-11 12:16:55]

On 2017/09/11 09:24:00, anton wrote:
> On 2017/09/08 12:25:31, sergei wrote:
> >
>
https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
> > File libadblockplus-android/jni/JniFilterEngine.cpp (right):
> > 
> >
>
https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
> > libadblockplus-android/jni/JniFilterEngine.cpp:57: static jboolean JNICALL
> > JniIsFirstRun(JNIEnv* env, jclass clazz, jlong ptr)
> > On 2017/09/08 10:19:18, anton wrote:
> > > On 2017/09/08 09:45:01, sergei wrote:
> > > > On 2017/09/06 06:21:27, anton wrote:
> > > > > i'd suggest to rename all `ptr` to `platformPtr` in all methods
> > > > 
> > > > I'm not sure that there is a real necessity in it because here `ptr`
plays
> a
> > > > role of an opaque pointer and since we are not working with it directly
> only
> > > > through GetFilterEngineRef I would propose to leave `ptr` argument in
> these
> > > > methods, but rename it only where its nature is actually used, namely in
> > > > GetFilterEngineRef and in JniUpdateFiltersAsync. What do you think about
> it?
> > > > BTW, just below we even use generic `engine` however we keep in mind the
> > > actual
> > > > kind of the engine.
> > > 
> > > i'd prefer it to be renamed as previously it was c++ filter engine ref
> > actually
> > > and now it's platform actually (JniPlatform instance to be more detailed).
> > This
> > > could introduce misunderstanding.
> > 
> > I would like to wait for the third opinion here. I think that the only
meaning
> > `ptr` is  carrying here is a native pointer held by Java class and it should
> not
> > show in each method what is actually behind it. Why should it generate a lot
> of
> > changes if we change the nature of this pointer if it's used as an opaque
> > integer value?
> > 
> >
>
https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
> > File
> libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java
> > (right):
> > 
> >
>
https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
> >
>
libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java:38:
> > FilterEngine(/* JniPlatform */long ptr)
> > On 2017/09/08 10:19:19, anton wrote:
> > > On 2017/09/08 09:45:01, sergei wrote:
> > > > On 2017/09/06 06:21:27, anton wrote:
> > > > > `public` here is required. Otherwise it's `package` so it's working
for
> > the
> > > > > classes from the same package only.
> > > > 
> > > > I actually wanted it, so FilterEngine can be created only by a class
from
> > this
> > > > package, namely only by Platform.
> > > 
> > > 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.
> > 
> > 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.
> > 
> >
>
https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
> > File libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java
> > (right):
> > 
> >
>
https://codereview.adblockplus.org/29536629/diff/29536630/libadblockplus-andr...
> > libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:59:
> > return new JsEngine(getJsEnginePtr(this.ptr));
> > On 2017/09/08 10:19:19, anton wrote:
> > > On 2017/09/08 09:45:02, sergei wrote:
> > > > On 2017/09/06 06:21:28, anton wrote:
> > > > > 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? 
> > > > 
> > > > 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?
> > > 
> > > 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.
> > 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.
> > 
> >
>
https://codereview.adblockplus.org/29536629/diff/29539610/libadblockplus-andr...
> > File libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java
> > (right):
> > 
> >
>
https://codereview.adblockplus.org/29536629/diff/29539610/libadblockplus-andr...
> > libadblockplus-android/src/org/adblockplus/libadblockplus/Platform.java:31:
//
> > If an interface parameter value is null then a default implementation is
> > On 2017/09/08 10:54:22, anton wrote:
> > > it would be better to have javadoc (/**, not just comment) here
> > 
> > Done.
> 
> LGTM

LGTM