Issue 3364 - Implement JNI bindings for IsDocument/ElemhideWhitelisted

Issue 3364 - Implement JNI bindings for IsDocument/ElemhideWhitelisted

[Felix Dahlke, 2015-12-04 15:43:20]

https://codereview.adblockplus.org/29331923/diff/29331932/jni/JniFilterEngine...
File jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29331923/diff/29331932/jni/JniFilterEngine...
jni/JniFilterEngine.cpp:314: static void JavaStringArrayIntoStringVector(JNIEnv*
env, jobjectArray jArray,
Nit: Seems like the name is inconsistent with the stuff we have in Utils,
shouldn't it be `JniJavaStringArrayToStringVector` (prefixed with Jni,
s/Into/To/)?

https://codereview.adblockplus.org/29331923/diff/29331932/src/org/adblockplus...
File src/org/adblockplus/android/ABPEngine.java (right):

https://codereview.adblockplus.org/29331923/diff/29331932/src/org/adblockplus...
src/org/adblockplus/android/ABPEngine.java:294: public boolean
isDocumentWhitelisted(final String url, final String[] referrerChainArray)
It's a bit unfortunate that this was called "referrerChainArray" above, because
it's actually the "documentUrls", same as in libadblockplus. The referrer chain
is one (crude!) way of determining the document URLs.

But let's keep it consistent with matches() above.

https://codereview.adblockplus.org/29331923/diff/29331932/src/org/adblockplus...
src/org/adblockplus/android/ABPEngine.java:306: if (!this.elemhideEnabled
This deserves a comment. Until now this was just a forwarding function, now it's
adding logic, logic that we would eventually like to see in
ElemHide.getSelectorsForDomain. We should create an issue for that and refer to
it here.

[René Jeschke, 2015-12-04 15:56:00]

New patch set up.

https://codereview.adblockplus.org/29331923/diff/29331932/jni/JniFilterEngine...
File jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29331923/diff/29331932/jni/JniFilterEngine...
jni/JniFilterEngine.cpp:314: static void JavaStringArrayIntoStringVector(JNIEnv*
env, jobjectArray jArray,
On 2015/12/04 15:43:20, Felix Dahlke wrote:
> Nit: Seems like the name is inconsistent with the stuff we have in Utils,
> shouldn't it be `JniJavaStringArrayToStringVector` (prefixed with Jni,
> s/Into/To/)?

Yes, this is because this is not a function that is a direct JNI entry point
(like the other Jni* functions).

And second, this functions transforms a 'Java String array' into a
'std::vector<std::string>', by putting the elements 'into' the given vector.

I would like to leave the name as is, or change 'Into' to 'To' if this is more
like it. :-)

https://codereview.adblockplus.org/29331923/diff/29331932/src/org/adblockplus...
File src/org/adblockplus/android/ABPEngine.java (right):

https://codereview.adblockplus.org/29331923/diff/29331932/src/org/adblockplus...
src/org/adblockplus/android/ABPEngine.java:294: public boolean
isDocumentWhitelisted(final String url, final String[] referrerChainArray)
On 2015/12/04 15:43:20, Felix Dahlke wrote:
> It's a bit unfortunate that this was called "referrerChainArray" above,
because
> it's actually the "documentUrls", same as in libadblockplus. The referrer
chain
> is one (crude!) way of determining the document URLs.
> 
> But let's keep it consistent with matches() above.

Done.

Remark: "matches above" uses also "referrerChainArray" :D

https://codereview.adblockplus.org/29331923/diff/29331932/src/org/adblockplus...
src/org/adblockplus/android/ABPEngine.java:306: if (!this.elemhideEnabled
On 2015/12/04 15:43:20, Felix Dahlke wrote:
> This deserves a comment. Until now this was just a forwarding function, now
it's
> adding logic, logic that we would eventually like to see in
> ElemHide.getSelectorsForDomain. We should create an issue for that and refer
to
> it here.

Nope, we don't need an issue for that. 'elemhideEnabled' is purely and solely
Adblock Plus for Android specific. This flag is here to ensure that ABPdroid is
still working as before (filtering elemhide rules while downloading and
returning 'no' filters) and that ASUS is able to use element hiding without
adding code.

I will add a comment, if you still want this.

[Felix Dahlke, 2015-12-04 16:37:36]

https://codereview.adblockplus.org/29331923/diff/29331932/jni/JniFilterEngine...
File jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29331923/diff/29331932/jni/JniFilterEngine...
jni/JniFilterEngine.cpp:314: static void JavaStringArrayIntoStringVector(JNIEnv*
env, jobjectArray jArray,
On 2015/12/04 15:55:59, René Jeschke wrote:
> On 2015/12/04 15:43:20, Felix Dahlke wrote:
> > Nit: Seems like the name is inconsistent with the stuff we have in Utils,
> > shouldn't it be `JniJavaStringArrayToStringVector` (prefixed with Jni,
> > s/Into/To/)?
> 
> Yes, this is because this is not a function that is a direct JNI entry point
> (like the other Jni* functions).
> 
> And second, this functions transforms a 'Java String array' into a
> 'std::vector<std::string>', by putting the elements 'into' the given vector.
> 
> I would like to leave the name as is, or change 'Into' to 'To' if this is more
> like it. :-)

I'd personally prefer "To" but I don't care that much.

https://codereview.adblockplus.org/29331923/diff/29331932/src/org/adblockplus...
File src/org/adblockplus/android/ABPEngine.java (right):

https://codereview.adblockplus.org/29331923/diff/29331932/src/org/adblockplus...
src/org/adblockplus/android/ABPEngine.java:294: public boolean
isDocumentWhitelisted(final String url, final String[] referrerChainArray)
On 2015/12/04 15:55:59, René Jeschke wrote:
> On 2015/12/04 15:43:20, Felix Dahlke wrote:
> > It's a bit unfortunate that this was called "referrerChainArray" above,
> because
> > it's actually the "documentUrls", same as in libadblockplus. The referrer
> chain
> > is one (crude!) way of determining the document URLs.
> > 
> > But let's keep it consistent with matches() above.
> 
> Done.
> 
> Remark: "matches above" uses also "referrerChainArray" :D

I don't get the impression you read the part where I wrote: "But let's keep it
consistent with matches() above." :D

https://codereview.adblockplus.org/29331923/diff/29331932/src/org/adblockplus...
src/org/adblockplus/android/ABPEngine.java:306: if (!this.elemhideEnabled
On 2015/12/04 15:55:59, René Jeschke wrote:
> On 2015/12/04 15:43:20, Felix Dahlke wrote:
> > This deserves a comment. Until now this was just a forwarding function, now
> it's
> > adding logic, logic that we would eventually like to see in
> > ElemHide.getSelectorsForDomain. We should create an issue for that and refer
> to
> > it here.
> 
> Nope, we don't need an issue for that. 'elemhideEnabled' is purely and solely
> Adblock Plus for Android specific. This flag is here to ensure that ABPdroid
is
> still working as before (filtering elemhide rules while downloading and
> returning 'no' filters) and that ASUS is able to use element hiding without
> adding code.
> 
> I will add a comment, if you still want this.

Sorry, went for the wrong line here, I meant the isDocumentWhitelisted() ||
isElemhideWhitelisted() check below, this one makes sense of course.

[René Jeschke, 2015-12-04 16:54:48]

New patch set up.

https://codereview.adblockplus.org/29331923/diff/29331932/jni/JniFilterEngine...
File jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29331923/diff/29331932/jni/JniFilterEngine...
jni/JniFilterEngine.cpp:314: static void JavaStringArrayIntoStringVector(JNIEnv*
env, jobjectArray jArray,
On 2015/12/04 16:37:36, Felix Dahlke wrote:
> On 2015/12/04 15:55:59, René Jeschke wrote:
> > On 2015/12/04 15:43:20, Felix Dahlke wrote:
> > > Nit: Seems like the name is inconsistent with the stuff we have in Utils,
> > > shouldn't it be `JniJavaStringArrayToStringVector` (prefixed with Jni,
> > > s/Into/To/)?
> > 
> > Yes, this is because this is not a function that is a direct JNI entry point
> > (like the other Jni* functions).
> > 
> > And second, this functions transforms a 'Java String array' into a
> > 'std::vector<std::string>', by putting the elements 'into' the given vector.
> > 
> > I would like to leave the name as is, or change 'Into' to 'To' if this is
more
> > like it. :-)
> 
> I'd personally prefer "To" but I don't care that much.

Done.

https://codereview.adblockplus.org/29331923/diff/29331932/src/org/adblockplus...
File src/org/adblockplus/android/ABPEngine.java (right):

https://codereview.adblockplus.org/29331923/diff/29331932/src/org/adblockplus...
src/org/adblockplus/android/ABPEngine.java:294: public boolean
isDocumentWhitelisted(final String url, final String[] referrerChainArray)
On 2015/12/04 16:37:36, Felix Dahlke wrote:
> On 2015/12/04 15:55:59, René Jeschke wrote:
> > On 2015/12/04 15:43:20, Felix Dahlke wrote:
> > > It's a bit unfortunate that this was called "referrerChainArray" above,
> > because
> > > it's actually the "documentUrls", same as in libadblockplus. The referrer
> > chain
> > > is one (crude!) way of determining the document URLs.
> > > 
> > > But let's keep it consistent with matches() above.
> > 
> > Done.
> > 
> > Remark: "matches above" uses also "referrerChainArray" :D
> 
> I don't get the impression you read the part where I wrote: "But let's keep it
> consistent with matches() above." :D

Frell ...



reverted :D

https://codereview.adblockplus.org/29331923/diff/29331932/src/org/adblockplus...
src/org/adblockplus/android/ABPEngine.java:306: if (!this.elemhideEnabled
On 2015/12/04 16:37:36, Felix Dahlke wrote:
> On 2015/12/04 15:55:59, René Jeschke wrote:
> > On 2015/12/04 15:43:20, Felix Dahlke wrote:
> > > This deserves a comment. Until now this was just a forwarding function,
now
> > it's
> > > adding logic, logic that we would eventually like to see in
> > > ElemHide.getSelectorsForDomain. We should create an issue for that and
refer
> > to
> > > it here.
> > 
> > Nope, we don't need an issue for that. 'elemhideEnabled' is purely and
solely
> > Adblock Plus for Android specific. This flag is here to ensure that ABPdroid
> is
> > still working as before (filtering elemhide rules while downloading and
> > returning 'no' filters) and that ASUS is able to use element hiding without
> > adding code.
> > 
> > I will add a comment, if you still want this.
> 
> Sorry, went for the wrong line here, I meant the isDocumentWhitelisted() ||
> isElemhideWhitelisted() check below, this one makes sense of course.

Added some comments here and there.

[Felix Dahlke, 2015-12-04 17:03:38]

https://codereview.adblockplus.org/29331923/diff/29331932/src/org/adblockplus...
File src/org/adblockplus/android/ABPEngine.java (right):

https://codereview.adblockplus.org/29331923/diff/29331932/src/org/adblockplus...
src/org/adblockplus/android/ABPEngine.java:306: if (!this.elemhideEnabled
On 2015/12/04 16:54:47, René Jeschke wrote:
> On 2015/12/04 16:37:36, Felix Dahlke wrote:
> > On 2015/12/04 15:55:59, René Jeschke wrote:
> > > On 2015/12/04 15:43:20, Felix Dahlke wrote:
> > > > This deserves a comment. Until now this was just a forwarding function,
> now
> > > it's
> > > > adding logic, logic that we would eventually like to see in
> > > > ElemHide.getSelectorsForDomain. We should create an issue for that and
> refer
> > > to
> > > > it here.
> > > 
> > > Nope, we don't need an issue for that. 'elemhideEnabled' is purely and
> solely
> > > Adblock Plus for Android specific. This flag is here to ensure that
ABPdroid
> > is
> > > still working as before (filtering elemhide rules while downloading and
> > > returning 'no' filters) and that ASUS is able to use element hiding
without
> > > adding code.
> > > 
> > > I will add a comment, if you still want this.
> > 
> > Sorry, went for the wrong line here, I meant the isDocumentWhitelisted() ||
> > isElemhideWhitelisted() check below, this one makes sense of course.
> 
> Added some comments here and there.

Nice, but that's still not what I meant :D I meant the fact that we are checking
isDocumentWhitelisted and isElemhideWhitelisted here.

However, considering that this is really ABP for Android and not really the
libadblockplus bindings, I guess that's fine here.

https://codereview.adblockplus.org/29331923/diff/29331968/src/org/adblockplus...
File src/org/adblockplus/android/AdblockPlus.java (right):

https://codereview.adblockplus.org/29331923/diff/29331968/src/org/adblockplus...
src/org/adblockplus/android/AdblockPlus.java:296: * re-enabled element hiding
but only enhanced/fixed the current implementation.
Typo, should be: "[...] as we did not yet re-enable element [...]"

[René Jeschke, 2015-12-04 17:44:25]

New patch set up.

https://codereview.adblockplus.org/29331923/diff/29331968/src/org/adblockplus...
File src/org/adblockplus/android/AdblockPlus.java (right):

https://codereview.adblockplus.org/29331923/diff/29331968/src/org/adblockplus...
src/org/adblockplus/android/AdblockPlus.java:296: * re-enabled element hiding
but only enhanced/fixed the current implementation.
On 2015/12/04 17:03:37, Felix Dahlke wrote:
> Typo, should be: "[...] as we did not yet re-enable element [...]"

Done.

[Felix Dahlke, 2015-12-04 17:48:57]

LGTM