Remove explicit JNI references by adding UrlRequest.readFromUploadChannel callback.

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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.net;
 
 import org.apache.http.conn.ConnectTimeoutException;
 import org.chromium.base.CalledByNative;
 import org.chromium.base.JNINamespace;
 
@@ skipping 17 matching lines @@
 public class UrlRequest {
     private static final class ContextLock {
     }
 
     private final UrlRequestContext mRequestContext;
     private final String mUrl;
     private final int mPriority;
     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;
     private volatile boolean mCanceled;
     private volatile boolean mRecycled;
     private volatile boolean mFinished;
     private boolean mHeadersAvailable;
     private String mContentType;
     private long mContentLength;
     private long mUploadContentLength;
@@ skipping 48 matching lines @@
      * Sets data to upload as part of a POST request.
      *
      * @param contentType MIME type of the post content or null if this is not a
      *            POST.
      * @param data The content that needs to be uploaded if this is a POST
      *            request.
      */
     public void setUploadData(String contentType, byte[] data) {
         synchronized (mLock) {
             validateNotStarted();
-            mPostBodyContentType = contentType;
-            mPostBody = data;
-            mPostBodyChannel = null;
+            mUploadContentType = contentType;
+            mUploadData = data;
+            mUploadChannel = null;
         }
     }
 
     /**
      * Sets a readable byte channel to upload as part of a POST request.
      *
      * @param contentType MIME type of the post content or null if this is not a
      *            POST request.
      * @param channel The channel to read to read upload data from if this is a
      *            POST request.
      * @param contentLength The length of data to upload.
      */
     public void setUploadChannel(String contentType,
             ReadableByteChannel channel, long contentLength) {
         synchronized (mLock) {
             validateNotStarted();
-            mPostBodyContentType = contentType;
-            mPostBodyChannel = channel;
+            mUploadContentType = contentType;
+            mUploadChannel = channel;
             mUploadContentLength = contentLength;
-            mPostBody = null;
+            mUploadData = null;
         }
     }
 
     public void setHttpMethod(String method) {
         validateNotStarted();
         if (!("PUT".equals(method) || "POST".equals(method))) {
             throw new IllegalArgumentException("Only PUT and POST are allowed.");
         }
         mMethod = method;
     }
 
     public WritableByteChannel getSink() {
         return mSink;
     }
 
     public void start() {
         synchronized (mLock) {
             if (mCanceled) {
                 return;
             }
 
             validateNotStarted();
             validateNotRecycled();
 
             mStarted = true;
 
             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";
             }
 
             if (method != null) {
                 nativeSetMethod(mUrlRequestPeer, method);
             }
 
             if (mHeaders != null && !mHeaders.isEmpty()) {
                 for (Entry<String, String> entry : mHeaders.entrySet()) {
                     nativeAddHeader(mUrlRequestPeer, entry.getKey(),
                                     entry.getValue());
               }
             }
 
             if (mAdditionalHeaders != null) {
                 for (Entry<String, String> entry :
                      mAdditionalHeaders.entrySet()) {
                     nativeAddHeader(mUrlRequestPeer, entry.getKey(),
                                     entry.getValue());
               }
             }
 
-            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);
           }
     }
 
     public void cancel() {
         synchronized (mLock) {
             if (mCanceled) {
                 return;
@@ skipping 148 matching lines @@
     @SuppressWarnings("unused")
     @CalledByNative
     private void onAppendResponseHeader(ResponseHeadersMap headersMap,
             String name, String value) {
         if (!headersMap.containsKey(name)) {
             headersMap.put(name, new ArrayList<String>());
         }
         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");
         }
     }
 
     private void validateNotStarted() {
         if (mStarted) {
             throw new IllegalStateException("Request already started");
         }
@@ skipping 14 matching lines @@
 
     private native void nativeAddHeader(long urlRequestPeer, String name,
             String value);
 
     private native void nativeSetMethod(long urlRequestPeer, String method);
 
     private native void nativeSetUploadData(long urlRequestPeer,
             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);
 
     private native void nativeCancel(long urlRequestPeer);
 
     private native void nativeDestroyRequestPeer(long urlRequestPeer);
 
     private native int nativeGetErrorCode(long urlRequestPeer);
 
     private native int nativeGetHttpStatusCode(long urlRequestPeer);
 
     private native String nativeGetErrorString(long urlRequestPeer);
 
     private native String nativeGetContentType(long urlRequestPeer);
 
     private native long nativeGetContentLength(long urlRequestPeer);
 
     private native String nativeGetHeader(long urlRequestPeer, String name);
 
     private native void nativeGetAllHeaders(long urlRequestPeer,
             ResponseHeadersMap headers);
 
     // Explicit class to work around JNI-generator generics confusion.
     private class ResponseHeadersMap extends HashMap<String, List<String> > {
     }
 }