jni: Allow nullptr to be converted to JavaRef.

jni: Allow nullptr to be converted to JavaRef.

Allow nullptr to be converted to JavaRef, not just JavaParamRef. This
avoids having to declare a ScopedJavaLocalRef just to pass null to a
function that takes JavaRef, and makes C++ "nullptr" and Java "null"
equivalent.

BUG=506850
R=yfriedman@chromium.org

Committed: https://crrev.com/8a47be66f340b94662c74544a30d546b49ad0bc3
Cr-Commit-Position: refs/heads/master@{#362966}

[Torne, 2015-12-02 18:15:51]

Followup to https://codereview.chromium.org/1481033002/ - after thinking about
it more and encountering a few more cases, it makes sense to allow nullptr to be
promoted to JavaRef as well, not just JavaParamRef. Having to write
"ScopedJavaLocalRef<jobject>()" just to pass null to a function that wants a
java object is pretty icky.

This does require exposing a public nonvirtual destructor on JavaRef (to avoid
introducing a vtable here and ruining all the beneficial inlining), but JavaRefs
are passed around by reference, not as pointers, and I can't think of any
plausible code that would result in invoking the wrong destructor on a
ScopedJava(Local|Global)Ref as a result of this.

I haven't made the default constructor public, because there should still be no
reason for someone to actually declare a variable of type JavaRef.

[Yaron, 2015-12-02 19:42:10]

lgtm - care to do a follow-up pass on current offenders?

https://codereview.chromium.org/1492703003/diff/1/base/android/scoped_java_ref.h
File base/android/scoped_java_ref.h (right):

https://codereview.chromium.org/1492703003/diff/1/base/android/scoped_java_re...
base/android/scoped_java_ref.h:45: // ALlow nullptr to be converted to JavaRef.
This avoids having to declare an
Allow

[Torne, 2015-12-03 11:58:32]

On 2015/12/02 19:42:10, Yaron wrote:
> lgtm - care to do a follow-up pass on current offenders?

I'll take a look, I'm not sure if there actually are many right now - the reason
I made this change was primarily to avoid *introducing* a bunch of cases of this
in the course of the JavaParamRef migration :)

>
https://codereview.chromium.org/1492703003/diff/1/base/android/scoped_java_ref.h
> File base/android/scoped_java_ref.h (right):
> 
>
https://codereview.chromium.org/1492703003/diff/1/base/android/scoped_java_re...
> base/android/scoped_java_ref.h:45: // ALlow nullptr to be converted to
JavaRef.
> This avoids having to declare an
> Allow

Fixed.

[Torne, 2015-12-03 11:58:36]

The CQ bit was checked by torne@chromium.org

[Torne, 2015-12-03 11:58:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1492703003/#ps20001
(title: "fix typo in comment")

[commit-bot: I haz the power, 2015-12-03 11:58:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1492703003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1492703003/20001

[commit-bot: I haz the power, 2015-12-03 13:08:09]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-03 13:09:16]

Description was changed from

==========
jni: Allow nullptr to be converted to JavaRef.

Allow nullptr to be converted to JavaRef, not just JavaParamRef. This
avoids having to declare a ScopedJavaLocalRef just to pass null to a
function that takes JavaRef, and makes C++ "nullptr" and Java "null"
equivalent.

BUG=506850
R=yfriedman@chromium.org
==========

to

==========
jni: Allow nullptr to be converted to JavaRef.

Allow nullptr to be converted to JavaRef, not just JavaParamRef. This
avoids having to declare a ScopedJavaLocalRef just to pass null to a
function that takes JavaRef, and makes C++ "nullptr" and Java "null"
equivalent.

BUG=506850
R=yfriedman@chromium.org

Committed: https://crrev.com/8a47be66f340b94662c74544a30d546b49ad0bc3
Cr-Commit-Position: refs/heads/master@{#362966}
==========

[commit-bot: I haz the power, 2015-12-03 13:09:16]

Patchset 2 (id:??) landed as
https://crrev.com/8a47be66f340b94662c74544a30d546b49ad0bc3
Cr-Commit-Position: refs/heads/master@{#362966}

[Torne, 2015-12-03 14:19:26]

On 2015/12/03 11:58:32, Torne wrote:
> On 2015/12/02 19:42:10, Yaron wrote:
> > lgtm - care to do a follow-up pass on current offenders?
> 
> I'll take a look, I'm not sure if there actually are many right now - the
reason
> I made this change was primarily to avoid *introducing* a bunch of cases of
this
> in the course of the JavaParamRef migration :)

D'oh. I had a look, and there are indeed not very many uses of this to pass null
to functions (though there are a few, I'll change them): but there are *loads*
of uses to return null from JNI functions, and those can't be satisfied just
with this change here, because those specifically return ScopedJavaLocalRef by
value, which I haven't added a nullptr_t conversion for :)

I guess there's no reason not to also allow nullptr conversion for local/global
refs, since they both already have a default constructor that makes a null
reference anyway.

While I'm thinking about this, it would probably also be beneficial if
ScopedJavaLocalRef (and global as well for symmetry) had move constructors/move
assignment operators, to avoid the compiler generating code that calls
NewLocalRef on one side and immediately DeleteLocalRef on the other side (in
cases where it can't do RVO for whatever reason).

I'm going to get the rest of my in-progress changes landed (the parameter
rewrites, etc), and will then take a long look at scoped_java_ref to see what
other improvements are possible there.

[Yaron, 2015-12-03 15:30:01]

On 2015/12/03 14:19:26, Torne wrote:
> On 2015/12/03 11:58:32, Torne wrote:
> > On 2015/12/02 19:42:10, Yaron wrote:
> > > lgtm - care to do a follow-up pass on current offenders?
> > 
> > I'll take a look, I'm not sure if there actually are many right now - the
> reason
> > I made this change was primarily to avoid *introducing* a bunch of cases of
> this
> > in the course of the JavaParamRef migration :)
> 
> D'oh. I had a look, and there are indeed not very many uses of this to pass
null
> to functions (though there are a few, I'll change them): but there are *loads*
> of uses to return null from JNI functions, and those can't be satisfied just
> with this change here, because those specifically return ScopedJavaLocalRef by
> value, which I haven't added a nullptr_t conversion for :)

Right - ok. That's actually the case I was thinking of :(
> 
> I guess there's no reason not to also allow nullptr conversion for
local/global
> refs, since they both already have a default constructor that makes a null
> reference anyway.
> 
> While I'm thinking about this, it would probably also be beneficial if
> ScopedJavaLocalRef (and global as well for symmetry) had move
constructors/move
> assignment operators, to avoid the compiler generating code that calls
> NewLocalRef on one side and immediately DeleteLocalRef on the other side (in
> cases where it can't do RVO for whatever reason).
> 
> I'm going to get the rest of my in-progress changes landed (the parameter
> rewrites, etc), and will then take a long look at scoped_java_ref to see what
> other improvements are possible there.
sgtm