Issue 1424 - Update libadblockplus-binaries to 6f79af14ad4e

Issue 1424 - Update libadblockplus-binaries to 6f79af14ad4e

[Felix Dahlke, 2014-09-22 04:53:32]

Please note that I have yet to push libadblockplus and libadblockplus-binaries
for this - https://issues.adblockplus.org/ticket/1296 is still under review.

There are two relevant changes I'm updating the code for here:

- We've introduced a ContentType enum
(https://issues.adblockplus.org/ticket/1286)
- I've renamed UpdaterCallback to UpdateCheckDoneCallback in
https://issues.adblockplus.org/ticket/1296

[René Jeschke, 2014-09-22 09:24:40]

On 2014/09/22 04:53:32, Felix H. Dahlke wrote:
> Please note that I have yet to push libadblockplus and libadblockplus-binaries
> for this - https://issues.adblockplus.org/ticket/1296 is still under review.
> 
> There are two relevant changes I'm updating the code for here:
> 
> - We've introduced a ContentType enum
> (https://issues.adblockplus.org/ticket/1286)
> - I've renamed UpdaterCallback to UpdateCheckDoneCallback in
> https://issues.adblockplus.org/ticket/1296

Why did you rename the class? Now the name differs from that used in
libadblockplus:

void ForceUpdateCheck(UpdaterCallback callback = 0);

Any specific reason why we want two different names for the same thing here?

[Felix Dahlke, 2014-09-22 12:43:00]

On 2014/09/22 09:24:40, René Jeschke wrote:
> On 2014/09/22 04:53:32, Felix H. Dahlke wrote:
> > Please note that I have yet to push libadblockplus and
libadblockplus-binaries
> > for this - https://issues.adblockplus.org/ticket/1296 is still under review.
> > 
> > There are two relevant changes I'm updating the code for here:
> > 
> > - We've introduced a ContentType enum
> > (https://issues.adblockplus.org/ticket/1286)
> > - I've renamed UpdaterCallback to UpdateCheckDoneCallback in
> > https://issues.adblockplus.org/ticket/1296
> 
> Why did you rename the class? Now the name differs from that used in
> libadblockplus:
> 
> void ForceUpdateCheck(UpdaterCallback callback = 0);
> 
> Any specific reason why we want two different names for the same thing here?

It has been renamed in libadblockplus, but the change is not pushed yet - see my
first comment.

[René Jeschke, 2014-09-22 12:55:02]

On 2014/09/22 12:43:00, Felix H. Dahlke wrote:
> On 2014/09/22 09:24:40, René Jeschke wrote:
> > On 2014/09/22 04:53:32, Felix H. Dahlke wrote:
> > > Please note that I have yet to push libadblockplus and
> libadblockplus-binaries
> > > for this - https://issues.adblockplus.org/ticket/1296 is still under
review.
> > > 
> > > There are two relevant changes I'm updating the code for here:
> > > 
> > > - We've introduced a ContentType enum
> > > (https://issues.adblockplus.org/ticket/1286)
> > > - I've renamed UpdaterCallback to UpdateCheckDoneCallback in
> > > https://issues.adblockplus.org/ticket/1296
> > 
> > Why did you rename the class? Now the name differs from that used in
> > libadblockplus:
> > 
> > void ForceUpdateCheck(UpdaterCallback callback = 0);
> > 
> > Any specific reason why we want two different names for the same thing here?
> 
> It has been renamed in libadblockplus, but the change is not pushed yet - see
my
> first comment.

Yep, sorry, my fault, so: Ok.

[René Jeschke, 2014-09-22 12:56:26]

http://codereview.adblockplus.org/6245218638626816/diff/5629499534213120/jni/...
File jni/JniFilterEngine.cpp (right):

http://codereview.adblockplus.org/6245218638626816/diff/5629499534213120/jni/...
jni/JniFilterEngine.cpp:37: jclass enumClass = env->FindClass("java/lang/Enum");
Could you please use "env->GetObjectClass(jContentType)" instead of the direct
env->FindClass?

http://codereview.adblockplus.org/6245218638626816/diff/5629499534213120/jni/...
jni/JniFilterEngine.cpp:38: jmethodID nameMethod = env->GetMethodID(enumClass,
"name",
Is this line break really necessary?

http://codereview.adblockplus.org/6245218638626816/diff/5629499534213120/jni/...
jni/JniFilterEngine.cpp:174: JniUpdateCheckDoneCallback* callback =
Are those new line breaks really needed?
(See also below)

[Felix Dahlke, 2014-09-22 13:20:16]

Thanks, new patch set up!

http://codereview.adblockplus.org/6245218638626816/diff/5629499534213120/jni/...
File jni/JniFilterEngine.cpp (right):

http://codereview.adblockplus.org/6245218638626816/diff/5629499534213120/jni/...
jni/JniFilterEngine.cpp:37: jclass enumClass = env->FindClass("java/lang/Enum");
On 2014/09/22 12:56:26, René Jeschke wrote:
> Could you please use "env->GetObjectClass(jContentType)" instead of the direct
> env->FindClass?

Done.

http://codereview.adblockplus.org/6245218638626816/diff/5629499534213120/jni/...
jni/JniFilterEngine.cpp:38: jmethodID nameMethod = env->GetMethodID(enumClass,
"name",
On 2014/09/22 12:56:26, René Jeschke wrote:
> Is this line break really necessary?

IMO yes - we have an 80 column rule in C++ code after all. We're not too strict
about enforcing it, but I personally tend to ensure all code I touch is fully
compliant.

http://codereview.adblockplus.org/6245218638626816/diff/5629499534213120/jni/...
jni/JniFilterEngine.cpp:174: JniUpdateCheckDoneCallback* callback =
On 2014/09/22 12:56:26, René Jeschke wrote:
> Are those new line breaks really needed?
> (See also below)

IMO yes, see above.

[René Jeschke, 2014-09-22 13:30:05]

LGTM

http://codereview.adblockplus.org/6245218638626816/diff/5629499534213120/jni/...
File jni/JniFilterEngine.cpp (right):

http://codereview.adblockplus.org/6245218638626816/diff/5629499534213120/jni/...
jni/JniFilterEngine.cpp:38: jmethodID nameMethod = env->GetMethodID(enumClass,
"name",
On 2014/09/22 13:20:16, Felix H. Dahlke wrote:
> On 2014/09/22 12:56:26, René Jeschke wrote:
> > Is this line break really necessary?
> 
> IMO yes - we have an 80 column rule in C++ code after all. We're not too
strict
> about enforcing it, but I personally tend to ensure all code I touch is fully
> compliant.

Ah, right. The Android project can be confusing when it comes to line-widths as
it features C++ and Java.