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.

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

Should we be using javadoc comments?  @param, @returns, etc?

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

Done.

+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private int readFromUploadChannel(ByteBuffer dst) {

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

nit:  Suggest dest instead of dst.  It's a bit more common.

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

Done.

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

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

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.

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

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?

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

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

+            }
+            return result;
+        } catch (IOException e) {
+            mSinkException = e;
+            try {
+                mUploadChannel.close();
+            } catch (IOException ignored) {
+                // Ignore this exception.
+            }
+            cancel();
+            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>> {
     }
 }