Remove explicit JNI references by adding UrlRequest.readFromUploadChannel callback.

components/cronet/android/java/src/org/chromium/net/UrlRequest.java

Index: components/cronet/android/java/src/org/chromium/net/UrlRequest.java
diff --git a/components/cronet/android/java/src/org/chromium/net/UrlRequest.java b/components/cronet/android/java/src/org/chromium/net/UrlRequest.java
index 1c20120b89556cf429797128a5db9195bdf029a8..9c899a3342e24562840fcd96719b02849c71b307 100644
--- a/components/cronet/android/java/src/org/chromium/net/UrlRequest.java
+++ b/components/cronet/android/java/src/org/chromium/net/UrlRequest.java
@@ -35,10 +35,10 @@ public class UrlRequest {
     private final Map<String, String> mHeaders;
     private final WritableByteChannel mSink;
     private Map<String, String> mAdditionalHeaders;
-    private String mPostBodyContentType;
+    private String mUploadContentType;
     private String mMethod;
-    private byte[] mPostBody;
-    private ReadableByteChannel mPostBodyChannel;
+    private byte[] mUploadData;
+    private ReadableByteChannel mUploadChannel;
     private WritableByteChannel mOutputChannel;
     private IOException mSinkException;
     private volatile boolean mStarted;
@@ -107,9 +107,9 @@ public class UrlRequest {
     public void setUploadData(String contentType, byte[] data) {
         synchronized (mLock) {
             validateNotStarted();
-            mPostBodyContentType = contentType;
-            mPostBody = data;
-            mPostBodyChannel = null;
+            mUploadContentType = contentType;
+            mUploadData = data;
+            mUploadChannel = null;
         }
     }
 
@@ -126,10 +126,10 @@ public class UrlRequest {
             ReadableByteChannel channel, long contentLength) {
         synchronized (mLock) {
             validateNotStarted();
-            mPostBodyContentType = contentType;
-            mPostBodyChannel = channel;
+            mUploadContentType = contentType;
+            mUploadChannel = channel;
             mUploadContentLength = contentLength;
-            mPostBody = null;
+            mUploadData = null;
         }
     }
 
@@ -158,8 +158,8 @@ public class UrlRequest {
 
             String method = mMethod;
             if (method == null &&
-                    ((mPostBody != null && mPostBody.length > 0) ||
-                      mPostBodyChannel != null)) {
+                    ((mUploadData != null && mUploadData.length > 0) ||
+                      mUploadChannel != null)) {
                 // Default to POST if there is data to upload but no method was
                 // specified.
                 method = "POST";
@@ -184,12 +184,12 @@ public class UrlRequest {
               }
             }
 
-            if (mPostBody != null && mPostBody.length > 0) {
-                nativeSetUploadData(mUrlRequestPeer, mPostBodyContentType,
-                                    mPostBody);
-            } else if (mPostBodyChannel != null) {
-                nativeSetUploadChannel(mUrlRequestPeer, mPostBodyContentType,
-                                       mPostBodyChannel, mUploadContentLength);
+            if (mUploadData != null && mUploadData.length > 0) {
+                nativeSetUploadData(mUrlRequestPeer, mUploadContentType,
+                                    mUploadData);
+            } else if (mUploadChannel != null) {
+                nativeSetUploadChannel(mUrlRequestPeer, mUploadContentType,
+                                       mUploadContentLength);
             }
 
             nativeStart(mUrlRequestPeer);
@@ -358,6 +358,27 @@ public class UrlRequest {
         headersMap.get(name).add(value);
     }
 
+    /**
+     * Reads data from |mUploadChannel| into |dst|. Returns -1 and closes
+     * the channel on error.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private int onReadUploadChannel(ByteBuffer dst) {

[mmenke, 2014/08/05 19:32:40]

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).

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

Done.

+        if (mUploadChannel == null || !mUploadChannel.isOpen())
+            return -1;
+        try {
+            int result = mUploadChannel.read(dst);
+            if (result < 0)
+                mUploadChannel.close();

[mmenke, 2014/08/05 19:32:40]

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?

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

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? 

+            return result;
+        } catch (IOException e) {
+            mSinkException = e;
+            cancel();

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

I'll close the channel here as well.

+            return -1;
+        }
+    }
+
     private void validateNotRecycled() {
         if (mRecycled) {
             throw new IllegalStateException("Accessing recycled request");
@@ -392,8 +413,7 @@ public class UrlRequest {
             String contentType, byte[] content);
 
     private native void nativeSetUploadChannel(long urlRequestPeer,
-            String contentType, ReadableByteChannel content,
-            long contentLength);
+            String contentType, long contentLength);
 
     private native void nativeStart(long urlRequestPeer);