Issue 1848 - Clean up local reference handling

Issue 1848 - Clean up local reference handling

[René Jeschke, 2015-01-23 12:32:16]

I did not find the need to really use Push-/PopLocalFrame so it's only single
reference RAII. Tried to keep the code changes as small as possible, though
there were some formatting issues due to our (_outdated_) 80 columns line
wrapping requirement :-D

[Felix Dahlke, 2015-02-03 05:22:49]

I'm not really happy we have to do this, but I don't see any better way. Chasing
down individual reference table overflows and fixing those seems worse.

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
File jni/JniEventCallback.cpp (right):

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
jni/JniEventCallback.cpp:44: *JniLocalReference<jclass>(*env,
I think I've seen at least three instances of pretty much this code - have you
thought about returning a JniLocalReference<jclass> from something like
JniCallbackBase::GetCallbackClass()?

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
File jni/JniLogSystem.cpp (right):

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
jni/JniLogSystem.cpp:58: default:
This was fine as it was actually - we don't indent the switch body.

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
File jni/JniUpdateAvailableCallback.cpp (right):

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
jni/JniUpdateAvailableCallback.cpp:46:
env->GetObjectClass(GetCallbackObject())), "updateAvailableCallback",
Just my opinion, but I'd find this easier to read if there was an explicit break
after the continued line:

*JniLocalReference<jclass>(*env,
    env->GetObjectClass(GetCallbackObject())),
"updateAvailableCallback", "(Ljava/lang/String;)V");

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
File jni/Utils.h (right):

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
jni/Utils.h:51: explicit JniLocalReference(JNIEnv* jniEnv, T object)
Doesn't need to be explicit since there's two arguments and no default values.

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/src/...
File src/org/adblockplus/android/AndroidWebRequest.java (right):

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/src/...
src/org/adblockplus/android/AndroidWebRequest.java:28: import
org.adblockplus.libadblockplus.ServerResponse.NsStatus;
This import reordering should rather go into a separate (noissue) commit.

[René Jeschke, 2015-02-03 13:51:26]

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
File jni/JniEventCallback.cpp (right):

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
jni/JniEventCallback.cpp:44: *JniLocalReference<jclass>(*env,
On 2015/02/03 05:22:50, Felix H. Dahlke wrote:
> I think I've seen at least three instances of pretty much this code - have you
> thought about returning a JniLocalReference<jclass> from something like
> JniCallbackBase::GetCallbackClass()?

Yeah, the first implementation supported this, but not this here because I do
not have a copy constructor. Implementing a copy constructor would also mean
implementing reference counting, which I thought of as a little bit of
over-engineering.

So, we should leave it like it is.

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
File jni/JniLogSystem.cpp (right):

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
jni/JniLogSystem.cpp:58: default:
On 2015/02/03 05:22:50, Felix H. Dahlke wrote:
> This was fine as it was actually - we don't indent the switch body.

We do this in libadblockplus, the old JNI code here also did it, and it also is
written down in the Mozilla Coding Style. So, still change?

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
File jni/JniUpdateAvailableCallback.cpp (right):

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
jni/JniUpdateAvailableCallback.cpp:46:
env->GetObjectClass(GetCallbackObject())), "updateAvailableCallback",
On 2015/02/03 05:22:50, Felix H. Dahlke wrote:
> Just my opinion, but I'd find this easier to read if there was an explicit
break
> after the continued line:
> 
> *JniLocalReference<jclass>(*env,
>     env->GetObjectClass(GetCallbackObject())),
> "updateAvailableCallback", "(Ljava/lang/String;)V");

Done.

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
File jni/Utils.h (right):

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
jni/Utils.h:51: explicit JniLocalReference(JNIEnv* jniEnv, T object)
On 2015/02/03 05:22:50, Felix H. Dahlke wrote:
> Doesn't need to be explicit since there's two arguments and no default values.

Done.

[Felix Dahlke, 2015-02-04 04:11:50]

LGTM as-is, but some wrapping nits :P

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
File jni/JniEventCallback.cpp (right):

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
jni/JniEventCallback.cpp:44: *JniLocalReference<jclass>(*env,
On 2015/02/03 13:51:26, René Jeschke wrote:
> On 2015/02/03 05:22:50, Felix H. Dahlke wrote:
> > I think I've seen at least three instances of pretty much this code - have
you
> > thought about returning a JniLocalReference<jclass> from something like
> > JniCallbackBase::GetCallbackClass()?
> 
> Yeah, the first implementation supported this, but not this here because I do
> not have a copy constructor. Implementing a copy constructor would also mean
> implementing reference counting, which I thought of as a little bit of
> over-engineering.
> 
> So, we should leave it like it is.

Good point, yeah.

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
File jni/JniLogSystem.cpp (right):

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
jni/JniLogSystem.cpp:58: default:
On 2015/02/03 13:51:26, René Jeschke wrote:
> On 2015/02/03 05:22:50, Felix H. Dahlke wrote:
> > This was fine as it was actually - we don't indent the switch body.
> 
> We do this in libadblockplus, the old JNI code here also did it, and it also
is
> written down in the Mozilla Coding Style. So, still change?

Oops, you're right :P But still, unrelated change here :P

http://codereview.adblockplus.org/4761138508070912/diff/5135032834326528/jni/...
File jni/JniUpdateCheckDoneCallback.cpp (right):

http://codereview.adblockplus.org/4761138508070912/diff/5135032834326528/jni/...
jni/JniUpdateCheckDoneCallback.cpp:46:
env->GetObjectClass(GetCallbackObject())), "updateCheckDoneCallback",
Since you changed this in JniUpdateAvailableCallback.cpp - same thing here I'd
say.

http://codereview.adblockplus.org/4761138508070912/diff/5135032834326528/jni/...
File jni/JniWebRequest.cpp (right):

http://codereview.adblockplus.org/4761138508070912/diff/5135032834326528/jni/...
jni/JniWebRequest.cpp:48: env->GetObjectClass(GetCallbackObject())), "httpGET",
Since you changed this in JniUpdateAvailableCallback.cpp - same thing here I'd
say.

[René Jeschke, 2015-02-04 12:02:47]

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
File jni/JniLogSystem.cpp (right):

http://codereview.adblockplus.org/4761138508070912/diff/5639274879778816/jni/...
jni/JniLogSystem.cpp:58: default:
On 2015/02/04 04:11:50, Felix H. Dahlke wrote:
> On 2015/02/03 13:51:26, René Jeschke wrote:
> > On 2015/02/03 05:22:50, Felix H. Dahlke wrote:
> > > This was fine as it was actually - we don't indent the switch body.
> > 
> > We do this in libadblockplus, the old JNI code here also did it, and it also
> is
> > written down in the Mozilla Coding Style. So, still change?
> 
> Oops, you're right :P But still, unrelated change here :P

Yep, you're right on this, too. Will up a 'Noissue - Formatting' codereview for
all of this. Reverted for now.

http://codereview.adblockplus.org/4761138508070912/diff/5135032834326528/jni/...
File jni/JniUpdateCheckDoneCallback.cpp (right):

http://codereview.adblockplus.org/4761138508070912/diff/5135032834326528/jni/...
jni/JniUpdateCheckDoneCallback.cpp:46:
env->GetObjectClass(GetCallbackObject())), "updateCheckDoneCallback",
On 2015/02/04 04:11:51, Felix H. Dahlke wrote:
> Since you changed this in JniUpdateAvailableCallback.cpp - same thing here I'd
> say.

Done.

http://codereview.adblockplus.org/4761138508070912/diff/5135032834326528/jni/...
File jni/JniWebRequest.cpp (right):

http://codereview.adblockplus.org/4761138508070912/diff/5135032834326528/jni/...
jni/JniWebRequest.cpp:48: env->GetObjectClass(GetCallbackObject())), "httpGET",
On 2015/02/04 04:11:51, Felix H. Dahlke wrote:
> Since you changed this in JniUpdateAvailableCallback.cpp - same thing here I'd
> say.

Done.

[Felix Dahlke, 2015-02-04 13:02:24]

LGTM