Remove explicit JNI references by adding UrlRequest.readFromUploadChannel callback.

Remove explicit JNI references by adding UrlRequest.readFromUploadChannel callback.

BUG=390267
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288139

[mef, 2014-08-05 17:18:39]

Please take a look, I've changed WrappedChannelElementReader to read channel
using OnReadUploadData callback instead of explicit JNI calls to
ReadableByteChannel.

WDYT?

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
File components/cronet/android/wrapped_channel_upload_element_reader.cc (left):

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/wrapped_channel_upload_element_reader.cc:84: if
(bytes_read <= 0) {
I imagine that |bytes_read| could temporarily be 0 if data is not ready yet.

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/wrapped_channel_upload_element_reader.cc:85:
env->CallVoidMethod(channel_, g_method_close);
Charles, why do we need to close the channel here?

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
File components/cronet/android/wrapped_channel_upload_element_reader.cc (right):

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/wrapped_channel_upload_element_reader.cc:49: int
bytes_read = delegate_->OnReadUploadData(buf, buf_length);
Should I post it to file thread in this CL? 
Also, should I post it here or inside the delegate?

[Charles, 2014-08-05 18:33:33]

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
File components/cronet/android/wrapped_channel_upload_element_reader.cc (left):

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/wrapped_channel_upload_element_reader.cc:85:
env->CallVoidMethod(channel_, g_method_close);
On 2014/08/05 17:18:39, mef wrote:
> Charles, why do we need to close the channel here?

Closing the channel is required because you need to ensure that if it's a file,
the bytes are written to disk.

[mmenke, 2014-08-05 19:32:41]

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/UrlRequest.java
(right):

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:367: private
int onReadUploadChannel(ByteBuffer dst) {
onX means generally means it's called in response to X, not that it does X, so I
believe this should be called something like readFromUploadChannel.  Same goes
for the delegate's method (And should update description as well).

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:373:
mUploadChannel.close();
We close but don't cancel in this error path, and we cancel but don't close in
the exception path.  Is there a reason for that?

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
File components/cronet/android/org_chromium_net_UrlRequest.cc (right):

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/org_chromium_net_UrlRequest.cc:84: virtual int
OnReadUploadData(net::IOBuffer* buf, int buf_length) {
OVERRIDE

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/org_chromium_net_UrlRequest.cc:86: jobject jbuf =
env->NewDirectByteBuffer(
jbuf seems to violate the C++ style guide - I'd suggest just writting out
java_buffer or java_byte_buffer, or somesuch.

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/org_chromium_net_UrlRequest.cc:90:
env->DeleteLocalRef(jbuf);
Is this call actually needed?  Seems like it's not done in a lot of code, though
we do this all over the place in this file.

Fine with it here, just wondering.

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
File components/cronet/android/wrapped_channel_upload_element_reader.cc (right):

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/wrapped_channel_upload_element_reader.cc:17: JNIEnv*
env,
I don't think we need env, or the dependency on jni.h any more.

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/wrapped_channel_upload_element_reader.cc:52: return
bytes_read;
BUG:  The return value is bytes read or a network error code.  In the Java, you
return -1 on some errors, or a Java error code instead.  Returning -1, in
particular, means ERR_IO_PENDING, which I don't think is what we want to be
returning on errors.

This looks to be a bug in the old code as well.

[mef, 2014-08-05 20:44:28]

Thanks, PTAL.

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/UrlRequest.java
(right):

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:367: private
int onReadUploadChannel(ByteBuffer dst) {
On 2014/08/05 19:32:40, mmenke wrote:
> onX means generally means it's called in response to X, not that it does X, so
I
> believe this should be called something like readFromUploadChannel.  Same goes
> for the delegate's method (And should update description as well).

Done.

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:373:
mUploadChannel.close();
On 2014/08/05 19:32:40, mmenke wrote:
> We close but don't cancel in this error path, and we cancel but don't close in
> the exception path.  Is there a reason for that?
Per conversation with clm@ the -1 means 'EOF' and we should close the channel to
release system resources. I guess here we should actually return 0 instead?

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:377:
cancel();
I'll close the channel here as well.

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
File components/cronet/android/org_chromium_net_UrlRequest.cc (right):

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/org_chromium_net_UrlRequest.cc:84: virtual int
OnReadUploadData(net::IOBuffer* buf, int buf_length) {
On 2014/08/05 19:32:41, mmenke wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/org_chromium_net_UrlRequest.cc:86: jobject jbuf =
env->NewDirectByteBuffer(
On 2014/08/05 19:32:41, mmenke wrote:
> jbuf seems to violate the C++ style guide - I'd suggest just writting out
> java_buffer or java_byte_buffer, or somesuch.

Done.

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/org_chromium_net_UrlRequest.cc:90:
env->DeleteLocalRef(jbuf);
On 2014/08/05 19:32:41, mmenke wrote:
> Is this call actually needed?  Seems like it's not done in a lot of code,
though
> we do this all over the place in this file.
> 
> Fine with it here, just wondering.
IIUIC this is explicit refcounting, so we have to release it, unless we hold on
to the reference or use some kind of scoped ref counter (I'll check if there is
one). 
Could you point to some places where it is not done for newly allocated objects?

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
File components/cronet/android/wrapped_channel_upload_element_reader.cc (right):

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/wrapped_channel_upload_element_reader.cc:17: JNIEnv*
env,
On 2014/08/05 19:32:41, mmenke wrote:
> I don't think we need env, or the dependency on jni.h any more.

Done.

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/wrapped_channel_upload_element_reader.cc:52: return
bytes_read;
On 2014/08/05 19:32:41, mmenke wrote:
> BUG:  The return value is bytes read or a network error code.  In the Java,
you
> return -1 on some errors, or a Java error code instead.  Returning -1, in
> particular, means ERR_IO_PENDING, which I don't think is what we want to be
> returning on errors.
> 
> This looks to be a bug in the old code as well.

Done.

[mmenke, 2014-08-05 21:08:02]

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
File components/cronet/android/org_chromium_net_UrlRequest.cc (right):

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/org_chromium_net_UrlRequest.cc:90:
env->DeleteLocalRef(jbuf);
On 2014/08/05 20:44:28, mef wrote:
> On 2014/08/05 19:32:41, mmenke wrote:
> > Is this call actually needed?  Seems like it's not done in a lot of code,
> though
> > we do this all over the place in this file.
> > 
> > Fine with it here, just wondering.
> IIUIC this is explicit refcounting, so we have to release it, unless we hold
on
> to the reference or use some kind of scoped ref counter (I'll check if there
is
> one). 
> Could you point to some places where it is not done for newly allocated
objects?

Hrm...I missed that ,ost use ScopedJavaLocalRef, which is probably what we
should use as well.  And some add them to other objects, which I guess take
ownership of the reference?  Not sure.

And that just leaves...um...net_string_util_icu_alternatives_android.  Oops.

https://codereview.chromium.org/442803002/diff/40001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/UrlRequest.java
(right):

https://codereview.chromium.org/442803002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:363: * the
channel on error.
Should we be using javadoc comments?  @param, @returns, etc?

https://codereview.chromium.org/442803002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:367: private
int readFromUploadChannel(ByteBuffer dst) {
nit:  Suggest dest instead of dst.  It's a bit more common.

https://codereview.chromium.org/442803002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:374: return
0;
Hrm...think this is actually a fail case, too.  We only read when we haven't
read up to the reported size, so if we read and get 0, we'll read again...and
then return -1 in the isOpen check.  Seems like it should be fine, though.

https://codereview.chromium.org/442803002/diff/40001/components/cronet/androi...
File components/cronet/android/org_chromium_net_UrlRequest.cc (right):

https://codereview.chromium.org/442803002/diff/40001/components/cronet/androi...
components/cronet/android/org_chromium_net_UrlRequest.cc:88: jobject jbuf =
env->NewDirectByteBuffer(
On 2014/08/05 20:44:28, mef wrote:
> On 2014/08/05 19:32:41, mmenke wrote:
> > jbuf seems to violate the C++ style guide - I'd suggest just writting out
> > java_buffer or java_byte_buffer, or somesuch.
> 
> Done.

You renamed the wrong one, though I agree with that rename as well.

[mef, 2014-08-05 21:38:45]

Thanks, PTAL.

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
File components/cronet/android/org_chromium_net_UrlRequest.cc (right):

https://codereview.chromium.org/442803002/diff/20001/components/cronet/androi...
components/cronet/android/org_chromium_net_UrlRequest.cc:90:
env->DeleteLocalRef(jbuf);
On 2014/08/05 21:08:02, mmenke wrote:
> On 2014/08/05 20:44:28, mef wrote:
> > On 2014/08/05 19:32:41, mmenke wrote:
> > > Is this call actually needed?  Seems like it's not done in a lot of code,
> > though
> > > we do this all over the place in this file.
> > > 
> > > Fine with it here, just wondering.
> > IIUIC this is explicit refcounting, so we have to release it, unless we hold
> on
> > to the reference or use some kind of scoped ref counter (I'll check if there
> is
> > one). 
> > Could you point to some places where it is not done for newly allocated
> objects?
> 
> Hrm...I missed that ,ost use ScopedJavaLocalRef, which is probably what we
> should use as well.  And some add them to other objects, which I guess take
> ownership of the reference?  Not sure.
> 
> And that just leaves...um...net_string_util_icu_alternatives_android.  Oops.

Done.

https://codereview.chromium.org/442803002/diff/40001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/UrlRequest.java
(right):

https://codereview.chromium.org/442803002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:363: * the
channel on error.
On 2014/08/05 21:08:02, mmenke wrote:
> Should we be using javadoc comments?  @param, @returns, etc?

Done.

https://codereview.chromium.org/442803002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:367: private
int readFromUploadChannel(ByteBuffer dst) {
On 2014/08/05 21:08:02, mmenke wrote:
> nit:  Suggest dest instead of dst.  It's a bit more common.

Done.

https://codereview.chromium.org/442803002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:374: return
0;
On 2014/08/05 21:08:02, mmenke wrote:
> Hrm...think this is actually a fail case, too.  We only read when we haven't
> read up to the reported size, so if we read and get 0, we'll read again...and
> then return -1 in the isOpen check.  Seems like it should be fine, though.
Yeah, in my testing current implementation doesn't attempt to read beyond the
|GetContentLength()| of UploadElementReader, so in normal file upload scenario
this is not triggered, which probably is not good?

https://codereview.chromium.org/442803002/diff/40001/components/cronet/androi...
File components/cronet/android/org_chromium_net_UrlRequest.cc (right):

https://codereview.chromium.org/442803002/diff/40001/components/cronet/androi...
components/cronet/android/org_chromium_net_UrlRequest.cc:88: jobject jbuf =
env->NewDirectByteBuffer(
On 2014/08/05 21:08:02, mmenke wrote:
> On 2014/08/05 20:44:28, mef wrote:
> > On 2014/08/05 19:32:41, mmenke wrote:
> > > jbuf seems to violate the C++ style guide - I'd suggest just writting out
> > > java_buffer or java_byte_buffer, or somesuch.
> > 
> > Done.
> 
> You renamed the wrong one, though I agree with that rename as well.

Done.

[mmenke, 2014-08-06 14:33:27]

LGTM

https://codereview.chromium.org/442803002/diff/40001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/UrlRequest.java
(right):

https://codereview.chromium.org/442803002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:374: return
0;
On 2014/08/05 21:38:45, mef wrote:
> On 2014/08/05 21:08:02, mmenke wrote:
> > Hrm...think this is actually a fail case, too.  We only read when we haven't
> > read up to the reported size, so if we read and get 0, we'll read
again...and
> > then return -1 in the isOpen check.  Seems like it should be fine, though.
> Yeah, in my testing current implementation doesn't attempt to read beyond the
> |GetContentLength()| of UploadElementReader, so in normal file upload scenario
> this is not triggered, which probably is not good?

Yea, the issue is just if we get the wrong length.  Anyways, seems fine.

[mef, 2014-08-07 18:34:43]

The CQ bit was checked by mef@chromium.org

[commit-bot: I haz the power, 2014-08-07 18:38:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mef@chromium.org/442803002/120001

[commit-bot: I haz the power, 2014-08-07 20:52:39]

Change committed as 288139