Allowing a Java object field to be null in a new JNI helper method.

Allowing a Java object field to be null in a new JNI helper method.

Java objects in the API should be allowed to be null in some cases.
Specifically, a null value for maxBitrateBps in RtpParameters.java
has a specific meaning and doesn't imply an error has occurred.

NOTRY=True

Committed: https://crrev.com/60631775fa067042b025b04f4307ea16dd6311b0
Cr-Commit-Position: refs/heads/master@{#12221}

[Taylor Brandstetter, 2016-03-31 21:44:48]

deadbeef@webrtc.org changed reviewers:
+ glaznev@webrtc.org

[Taylor Brandstetter, 2016-03-31 21:44:48]

PTAL.

[AlexG, 2016-03-31 23:06:27]

On 2016/03/31 21:44:48, Taylor Brandstetter wrote:
> PTAL.

Why not change maxBitrateBps type to int instead of Integer and keep this check?

[Taylor Brandstetter, 2016-04-01 00:45:12]

On 2016/03/31 23:06:27, AlexG wrote:
> On 2016/03/31 21:44:48, Taylor Brandstetter wrote:
> > PTAL.
> 
> Why not change maxBitrateBps type to int instead of Integer and keep this
check?

Because a null value means "remove the bitrate limit, if one exists". Before we
were using -1 or 0 to do this, but we prefer nullable/optional values over
values with special case constants.

[AlexG, 2016-04-01 00:48:24]

On 2016/04/01 00:45:12, Taylor Brandstetter wrote:
> On 2016/03/31 23:06:27, AlexG wrote:
> > On 2016/03/31 21:44:48, Taylor Brandstetter wrote:
> > > PTAL.
> > 
> > Why not change maxBitrateBps type to int instead of Integer and keep this
> check?
> 
> Because a null value means "remove the bitrate limit, if one exists". Before
we
> were using -1 or 0 to do this, but we prefer nullable/optional values over
> values with special case constants.

Add a boolean flag for "remove the bitrate limit, if one exists".
Having Integer value equal to null have a special non documented meaning looks
confusing

[Taylor Brandstetter, 2016-04-01 00:52:35]

Description was changed from

==========
Allowing a Java object field to be null in the JNI helper method.

Java objects in the API should be allowed to be null.
Specifically, a null value for maxBitrateBps in RtpParameters.java
has a specific meaning and doesn't imply an error has occurred.
==========

to

==========
Allowing a Java object field to be null in the JNI helper method.

Java objects in the API should be allowed to be null.
Specifically, a null value for maxBitrateBps in RtpParameters.java
has a specific meaning and doesn't imply an error has occurred.
==========

[Taylor Brandstetter, 2016-04-01 00:52:35]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org, skvlad@webrtc.org

[Taylor Brandstetter, 2016-04-01 01:01:40]

On 2016/04/01 00:48:24, AlexG wrote:
> On 2016/04/01 00:45:12, Taylor Brandstetter wrote:
> > On 2016/03/31 23:06:27, AlexG wrote:
> > > On 2016/03/31 21:44:48, Taylor Brandstetter wrote:
> > > > PTAL.
> > > 
> > > Why not change maxBitrateBps type to int instead of Integer and keep this
> > check?
> > 
> > Because a null value means "remove the bitrate limit, if one exists". Before
> we
> > were using -1 or 0 to do this, but we prefer nullable/optional values over
> > values with special case constants.
> 
> Add a boolean flag for "remove the bitrate limit, if one exists".
> Having Integer value equal to null have a special non documented meaning looks
> confusing

If the issue is lack of documentation, could we just add comments where
appropriate? The nullable value matches the "rtc::Optional" used underneath, and
the optional dictionary field in the JavaScript API. I think the current design
is pretty intuitive and simple to use, personally. CC'ing skvlad and pthatcher
who were involved with the bitrate limit CL.

[AlexG, 2016-04-01 01:29:54]

On 2016/04/01 01:01:40, Taylor Brandstetter wrote:
> On 2016/04/01 00:48:24, AlexG wrote:
> > On 2016/04/01 00:45:12, Taylor Brandstetter wrote:
> > > On 2016/03/31 23:06:27, AlexG wrote:
> > > > On 2016/03/31 21:44:48, Taylor Brandstetter wrote:
> > > > > PTAL.
> > > > 
> > > > Why not change maxBitrateBps type to int instead of Integer and keep
this
> > > check?
> > > 
> > > Because a null value means "remove the bitrate limit, if one exists".
Before
> > we
> > > were using -1 or 0 to do this, but we prefer nullable/optional values over
> > > values with special case constants.
> > 
> > Add a boolean flag for "remove the bitrate limit, if one exists".
> > Having Integer value equal to null have a special non documented meaning
looks
> > confusing
> 
> If the issue is lack of documentation, could we just add comments where
> appropriate? The nullable value matches the "rtc::Optional" used underneath,
and
> the optional dictionary field in the JavaScript API. I think the current
design
> is pretty intuitive and simple to use, personally. CC'ing skvlad and pthatcher
> who were involved with the bitrate limit CL.

Then may be add a separate function - GetNullableObjectField which assume that
field may be null and use it for maxBitrateBps values reading.
For all other non nullable fields it is still worth to have this non null check.

[skvlad, 2016-04-01 01:39:00]

On 2016/04/01 01:29:54, AlexG wrote:
> On 2016/04/01 01:01:40, Taylor Brandstetter wrote:
> > On 2016/04/01 00:48:24, AlexG wrote:
> > > On 2016/04/01 00:45:12, Taylor Brandstetter wrote:
> > > > On 2016/03/31 23:06:27, AlexG wrote:
> > > > > On 2016/03/31 21:44:48, Taylor Brandstetter wrote:
> > > > > > PTAL.
> > > > > 
> > > > > Why not change maxBitrateBps type to int instead of Integer and keep
> this
> > > > check?
> > > > 
> > > > Because a null value means "remove the bitrate limit, if one exists".
> Before
> > > we
> > > > were using -1 or 0 to do this, but we prefer nullable/optional values
over
> > > > values with special case constants.
> > > 
> > > Add a boolean flag for "remove the bitrate limit, if one exists".
> > > Having Integer value equal to null have a special non documented meaning
> looks
> > > confusing
> > 
> > If the issue is lack of documentation, could we just add comments where
> > appropriate? The nullable value matches the "rtc::Optional" used underneath,
> and
> > the optional dictionary field in the JavaScript API. I think the current
> design
> > is pretty intuitive and simple to use, personally. CC'ing skvlad and
pthatcher
> > who were involved with the bitrate limit CL.
> 
> Then may be add a separate function - GetNullableObjectField which assume that
> field may be null and use it for maxBitrateBps values reading.
> For all other non nullable fields it is still worth to have this non null
check.

If the method indeed checks for a Java null value, it appears that just
comparing the jobject value to the C++ nullptr isn't always enough - in
particular it doesn't work for weak references. The correct way to check for a
null value is to do what the IsNull function does - call
jni->IsSameObject(your_jobject, nullptr).
Other than that, I think an exception check (which is never expected to happen
while reading a field value) and a null check (which just happens to be not a
valid value in most of the WebRTC Java API, but is perfectly normal in Java)
really belong in separate functions. Perhaps we can rename the current one to
GetNonNullObjectField, and have GetObjectField not do the check?

Ideally I think we should throw a Java exception when a wrong value is passed in
- such as null when it isn't expected - instead of crashing the process with
little feedback to the application developer.

[Taylor Brandstetter, 2016-04-01 18:19:04]

> > Then may be add a separate function - GetNullableObjectField which assume
that
> > field may be null and use it for maxBitrateBps values reading.
> > For all other non nullable fields it is still worth to have this non null
> check.
> 
> If the method indeed checks for a Java null value, it appears that just
> comparing the jobject value to the C++ nullptr isn't always enough - in
> particular it doesn't work for weak references. The correct way to check for a
> null value is to do what the IsNull function does - call
> jni->IsSameObject(your_jobject, nullptr).
> Other than that, I think an exception check (which is never expected to happen
> while reading a field value) and a null check (which just happens to be not a
> valid value in most of the WebRTC Java API, but is perfectly normal in Java)
> really belong in separate functions. Perhaps we can rename the current one to
> GetNonNullObjectField, and have GetObjectField not do the check?
> 
> Ideally I think we should throw a Java exception when a wrong value is passed
in
> - such as null when it isn't expected - instead of crashing the process with
> little feedback to the application developer.

I agree that the null checks should throw a Java exception. However, for now, I
think having separate "GetNonNullableObjectField" and "GetNullableObjectField"
methods sounds fine. Since GetObjectField has been around for a while, I'd be ok
with leaving it how it is and making "GetNullableObjectField" the new method. We
can leave a comment in jni_helpers.h explaining that GetObjectField checks for
null.

[Taylor Brandstetter, 2016-04-01 18:24:07]

Description was changed from

==========
Allowing a Java object field to be null in the JNI helper method.

Java objects in the API should be allowed to be null.
Specifically, a null value for maxBitrateBps in RtpParameters.java
has a specific meaning and doesn't imply an error has occurred.
==========

to

==========
Allowing a Java object field to be null in a new JNI helper method.

Java objects in the API should be allowed to be null in some cases.
Specifically, a null value for maxBitrateBps in RtpParameters.java
has a specific meaning and doesn't imply an error has occurred.
==========

[AlexG, 2016-04-01 18:38:27]

On 2016/04/01 18:19:04, Taylor Brandstetter wrote:
> > > Then may be add a separate function - GetNullableObjectField which assume
> that
> > > field may be null and use it for maxBitrateBps values reading.
> > > For all other non nullable fields it is still worth to have this non null
> > check.
> > 
> > If the method indeed checks for a Java null value, it appears that just
> > comparing the jobject value to the C++ nullptr isn't always enough - in
> > particular it doesn't work for weak references. The correct way to check for
a
> > null value is to do what the IsNull function does - call
> > jni->IsSameObject(your_jobject, nullptr).
> > Other than that, I think an exception check (which is never expected to
happen
> > while reading a field value) and a null check (which just happens to be not
a
> > valid value in most of the WebRTC Java API, but is perfectly normal in Java)
> > really belong in separate functions. Perhaps we can rename the current one
to
> > GetNonNullObjectField, and have GetObjectField not do the check?
> > 
> > Ideally I think we should throw a Java exception when a wrong value is
passed
> in
> > - such as null when it isn't expected - instead of crashing the process with
> > little feedback to the application developer.
> 
> I agree that the null checks should throw a Java exception. However, for now,
I
> think having separate "GetNonNullableObjectField" and "GetNullableObjectField"
> methods sounds fine. Since GetObjectField has been around for a while, I'd be
ok
> with leaving it how it is and making "GetNullableObjectField" the new method.
We
> can leave a comment in jni_helpers.h explaining that GetObjectField checks for
> null.

Sounds good.

[Taylor Brandstetter, 2016-04-01 22:11:43]

Ok, I made the modifications based on our discussion.

[AlexG, 2016-04-01 22:57:13]

lgtm

[Taylor Brandstetter, 2016-04-01 23:28:00]

Description was changed from

==========
Allowing a Java object field to be null in a new JNI helper method.

Java objects in the API should be allowed to be null in some cases.
Specifically, a null value for maxBitrateBps in RtpParameters.java
has a specific meaning and doesn't imply an error has occurred.
==========

to

==========
Allowing a Java object field to be null in a new JNI helper method.

Java objects in the API should be allowed to be null in some cases.
Specifically, a null value for maxBitrateBps in RtpParameters.java
has a specific meaning and doesn't imply an error has occurred.
NOTRY=true
==========

[Taylor Brandstetter, 2016-04-01 23:28:40]

Description was changed from

==========
Allowing a Java object field to be null in a new JNI helper method.

Java objects in the API should be allowed to be null in some cases.
Specifically, a null value for maxBitrateBps in RtpParameters.java
has a specific meaning and doesn't imply an error has occurred.
NOTRY=true
==========

to

==========
Allowing a Java object field to be null in a new JNI helper method.

Java objects in the API should be allowed to be null in some cases.
Specifically, a null value for maxBitrateBps in RtpParameters.java
has a specific meaning and doesn't imply an error has occurred.

NOTRY=True
==========

[Taylor Brandstetter, 2016-04-04 17:19:21]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2016-04-04 17:19:26]

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

[commit-bot: I haz the power, 2016-04-04 17:21:04]

Description was changed from

==========
Allowing a Java object field to be null in a new JNI helper method.

Java objects in the API should be allowed to be null in some cases.
Specifically, a null value for maxBitrateBps in RtpParameters.java
has a specific meaning and doesn't imply an error has occurred.

NOTRY=True
==========

to

==========
Allowing a Java object field to be null in a new JNI helper method.

Java objects in the API should be allowed to be null in some cases.
Specifically, a null value for maxBitrateBps in RtpParameters.java
has a specific meaning and doesn't imply an error has occurred.

NOTRY=True
==========

[commit-bot: I haz the power, 2016-04-04 17:21:05]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-04 17:27:36]

Description was changed from

==========
Allowing a Java object field to be null in a new JNI helper method.

Java objects in the API should be allowed to be null in some cases.
Specifically, a null value for maxBitrateBps in RtpParameters.java
has a specific meaning and doesn't imply an error has occurred.

NOTRY=True
==========

to

==========
Allowing a Java object field to be null in a new JNI helper method.

Java objects in the API should be allowed to be null in some cases.
Specifically, a null value for maxBitrateBps in RtpParameters.java
has a specific meaning and doesn't imply an error has occurred.

NOTRY=True

Committed: https://crrev.com/60631775fa067042b025b04f4307ea16dd6311b0
Cr-Commit-Position: refs/heads/master@{#12221}
==========

[commit-bot: I haz the power, 2016-04-04 17:27:37]

Patchset 3 (id:??) landed as
https://crrev.com/60631775fa067042b025b04f4307ea16dd6311b0
Cr-Commit-Position: refs/heads/master@{#12221}

[pthatcher1, 2016-04-11 19:39:29]

https://codereview.webrtc.org/1853523002/diff/40001/webrtc/api/java/jni/jni_h...
File webrtc/api/java/jni/jni_helpers.cc (right):

https://codereview.webrtc.org/1853523002/diff/40001/webrtc/api/java/jni/jni_h...
webrtc/api/java/jni/jni_helpers.cc:178: CHECK_EXCEPTION(jni) << "error during
GetObjectField";
Can't you replace this with GetNullableObjectField?

https://codereview.webrtc.org/1853523002/diff/40001/webrtc/api/java/jni/jni_h...
File webrtc/api/java/jni/jni_helpers.h (right):

https://codereview.webrtc.org/1853523002/diff/40001/webrtc/api/java/jni/jni_h...
webrtc/api/java/jni/jni_helpers.h:67: // Throws an exception if the object field
is null.
Doesn't the code actually crash?

https://codereview.webrtc.org/1853523002/diff/40001/webrtc/api/java/jni/jni_h...
webrtc/api/java/jni/jni_helpers.h:68: jobject GetObjectField(JNIEnv* jni,
jobject object, jfieldID id);
Can we rename this one GetNonNullObjectField to make it clear things may throw
an exception (or crash)?

[Taylor Brandstetter, 2016-04-11 23:13:38]

https://codereview.webrtc.org/1853523002/diff/40001/webrtc/api/java/jni/jni_h...
File webrtc/api/java/jni/jni_helpers.cc (right):

https://codereview.webrtc.org/1853523002/diff/40001/webrtc/api/java/jni/jni_h...
webrtc/api/java/jni/jni_helpers.cc:178: CHECK_EXCEPTION(jni) << "error during
GetObjectField";
On 2016/04/11 19:39:29, pthatcher1 wrote:
> Can't you replace this with GetNullableObjectField?

True.

https://codereview.webrtc.org/1853523002/diff/40001/webrtc/api/java/jni/jni_h...
File webrtc/api/java/jni/jni_helpers.h (right):

https://codereview.webrtc.org/1853523002/diff/40001/webrtc/api/java/jni/jni_h...
webrtc/api/java/jni/jni_helpers.h:67: // Throws an exception if the object field
is null.
On 2016/04/11 19:39:29, pthatcher1 wrote:
> Doesn't the code actually crash?  

Oh yeah. It calls abort(). I'll make a CL to fix this comment.

https://codereview.webrtc.org/1853523002/diff/40001/webrtc/api/java/jni/jni_h...
webrtc/api/java/jni/jni_helpers.h:68: jobject GetObjectField(JNIEnv* jni,
jobject object, jfieldID id);
On 2016/04/11 19:39:29, pthatcher1 wrote:
> Can we rename this one GetNonNullObjectField to make it clear things may throw
> an exception (or crash)?

Since you, Vlad and I prefer that naming, it seems worth doing. I'll do it in
the new CL.