Merge UrlRequest.java into ChromiumUrlRequest.java

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

Index: components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
diff --git a/components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java b/components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
index a643a1456230dc0d57adbba85a7644b460518b5a..e264ce1c75c9b9742ffa19110372710d73a3af2e 100644
--- a/components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
+++ b/components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
@@ -4,35 +4,63 @@
 
 package org.chromium.net;
 
+import org.apache.http.conn.ConnectTimeoutException;
+import org.chromium.base.CalledByNative;
+import org.chromium.base.JNINamespace;
+
 import java.io.IOException;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.net.UnknownHostException;
 import java.nio.ByteBuffer;
+import java.nio.channels.ReadableByteChannel;
 import java.nio.channels.WritableByteChannel;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 
 /**
  * Network request using the native http stack implementation.
  */
-public class ChromiumUrlRequest extends UrlRequest implements HttpUrlRequest {
-
+@JNINamespace("cronet")
+public class ChromiumUrlRequest implements HttpUrlRequest {
+    /**
+     * Native adapter object, owned by UrlRequest.
+     */
+    private long mUrlRequestAdapter;
+    private final ChromiumUrlRequestContext 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 mUploadContentType;
+    private String mMethod;
+    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 mUploadContentLength;
     private final HttpUrlRequestListener mListener;
-
     private boolean mBufferFullResponse;
-
     private long mOffset;
-
     private long mContentLength;
-
     private long mContentLengthLimit;
-
     private boolean mCancelIfContentLengthOverLimit;
-
     private boolean mContentLengthOverLimit;
-
     private boolean mSkippingToOffset;
-
     private long mSize;
+    private final Object mLock;
 
-    public ChromiumUrlRequest(UrlRequestContext requestContext,
+    public ChromiumUrlRequest(ChromiumUrlRequestContext requestContext,
             String url, int priority, Map<String, String> headers,
             HttpUrlRequestListener listener) {
         this(requestContext, url, priority, headers,
@@ -40,29 +68,36 @@ public class ChromiumUrlRequest extends UrlRequest implements HttpUrlRequest {
         mBufferFullResponse = true;
     }
 
-    public ChromiumUrlRequest(UrlRequestContext requestContext,
+    /**
+     * Constructor.
+     *
+     * @param requestContext The context.
+     * @param url The URL.
+     * @param priority Request priority, e.g. {@link #REQUEST_PRIORITY_MEDIUM}.
+     * @param headers HTTP headers.
+     * @param sink The output channel into which downloaded content will be
+     *            written.
+     */
+    public ChromiumUrlRequest(ChromiumUrlRequestContext requestContext,
             String url, int priority, Map<String, String> headers,
             WritableByteChannel sink, HttpUrlRequestListener listener) {
-        super(requestContext, url, convertRequestPriority(priority), headers,
-                sink);
-        mListener = listener;
-    }
-
-    private static int convertRequestPriority(int priority) {
-        switch (priority) {
-            case HttpUrlRequest.REQUEST_PRIORITY_IDLE:
-                return UrlRequestPriority.IDLE;
-            case HttpUrlRequest.REQUEST_PRIORITY_LOWEST:
-                return UrlRequestPriority.LOWEST;
-            case HttpUrlRequest.REQUEST_PRIORITY_LOW:
-                return UrlRequestPriority.LOW;
-            case HttpUrlRequest.REQUEST_PRIORITY_MEDIUM:
-                return UrlRequestPriority.MEDIUM;
-            case HttpUrlRequest.REQUEST_PRIORITY_HIGHEST:
-                return UrlRequestPriority.HIGHEST;
-            default:
-                return UrlRequestPriority.MEDIUM;
+        if (requestContext == null) {
+            throw new NullPointerException("Context is required");
         }
+        if (url == null) {
+            throw new NullPointerException("URL is required");
+        }
+        mRequestContext = requestContext;
+        mUrl = url;
+        mPriority = convertRequestPriority(priority);
+        mHeaders = headers;
+        mSink = sink;
+        mLock = new Object();
+        mUrlRequestAdapter = nativeCreateRequestAdapter(
+                mRequestContext.getChromiumUrlRequestContextAdapter(),
+                mUrl,
+                mPriority);

[mmenke, 2014/08/13 15:33:50]

Hrm...Not sure about the android style guide, but there are a lot of values
we're implicitly relying on the constructors of (mOffset to 0,
mBufferFullResponse to false, etc).

[mef, 2014/08/13 18:52:41]

Acknowledged.

+        mListener = listener;
     }
 
     @Override
@@ -73,6 +108,10 @@ public class ChromiumUrlRequest extends UrlRequest implements HttpUrlRequest {
         }
     }
 
+    /**
+     * Content length as reported by the server. May be -1 or incorrect if the
+     * server returns the wrong number, which happens even with Google servers.

[mmenke, 2014/08/13 15:33:50]

This doesn't seem quite right.  Maybe "The compressed content length as reported
by the server.  May be -1 if the server did not provide a length.  Some servers
may also report the wrong number.  Since this is the compressed content length,
and only uncompressed content is returned by the consumer, the consumer should
not rely on this value."

I'm not really sure what the intended use of this function is.  Maybe to
determine if something is likely to be a large/slow download?

[mmenke, 2014/08/13 15:33:51]

Should we add Javadoc style comments here?  @returns and all that stuff?  Fine
with putting it off for later, or an answer of "Maybe?"

[mef, 2014/08/13 18:52:41]

Done.

[mef, 2014/08/13 18:52:41]

I think so? I think that ideally (or possibly absolutely) ALL public methods
should have Javadoc comments.

+     */
     @Override
     public long getContentLength() {
         return mContentLength;
@@ -85,10 +124,238 @@ public class ChromiumUrlRequest extends UrlRequest implements HttpUrlRequest {
     }
 
     @Override
+    public int getHttpStatusCode() {
+        int httpStatusCode = nativeGetHttpStatusCode(mUrlRequestAdapter);
+
+        // TODO(mef): Investigate the following:
+        // If we have been able to successfully resume a previously interrupted
+        // download,
+        // the status code will be 206, not 200. Since the rest of the
+        // application is
+        // expecting 200 to indicate success, we need to fake it.
+        if (httpStatusCode == 206) {
+            httpStatusCode = 200;
+        }
+        return httpStatusCode;
+    }
+
+    /**
+     * Returns an exception if any, or null if the request was completed
+     * successfully.
+     */
+    @Override
+    public IOException getException() {
+        if (mSinkException != null) {
+            return mSinkException;
+        }
+
+        validateNotRecycled();
+
+        int errorCode = nativeGetErrorCode(mUrlRequestAdapter);
+        switch (errorCode) {
+            case ChromiumUrlRequestError.SUCCESS:
+                if (mContentLengthOverLimit) {
+                    return new ResponseTooLargeException();
+                }
+                return null;
+            case ChromiumUrlRequestError.UNKNOWN:
+                return new IOException(
+                        nativeGetErrorString(mUrlRequestAdapter));
+            case ChromiumUrlRequestError.MALFORMED_URL:
+                return new MalformedURLException("Malformed URL: " + mUrl);
+            case ChromiumUrlRequestError.CONNECTION_TIMED_OUT:
+                return new ConnectTimeoutException("Connection timed out");
+            case ChromiumUrlRequestError.UNKNOWN_HOST:
+                String host;
+                try {
+                    host = new URL(mUrl).getHost();
+                } catch (MalformedURLException e) {
+                    host = mUrl;
+                }
+                return new UnknownHostException("Unknown host: " + host);
+            default:
+                throw new IllegalStateException(
+                        "Unrecognized error code: " + errorCode);
+        }
+    }
+
+    @Override
+    public ByteBuffer getByteBuffer() {
+        return ((ChunkedWritableByteChannel)getSink()).getByteBuffer();
+    }
+
+    @Override
+    public byte[] getResponseAsBytes() {
+        return ((ChunkedWritableByteChannel)getSink()).getBytes();
+    }
+
+    /**
+     * Adds a request header.
+     */
+    public void addHeader(String header, String value) {

[mmenke, 2014/08/13 15:33:50]

Should this be synchronized?  Heck, should all setters be synchronized?  I
eagerly await the day we move to a more reasonable threading model.

[mef, 2014/08/13 18:52:41]

Acknowledged. I'd prefer to change it in separate CL though (just to keep this
one as simple as possible).

+        validateNotStarted();
+        if (mAdditionalHeaders == null) {
+            mAdditionalHeaders = new HashMap<String, String>();
+        }
+        mAdditionalHeaders.put(header, value);
+    }
+
+    /**
+     * 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

[mmenke, 2014/08/13 15:33:49]

nit:  post -> POST

[mef, 2014/08/13 18:52:41]

Done.

+     *            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();
+            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

[mmenke, 2014/08/13 15:33:49]

nit POST?

[mef, 2014/08/13 18:52:41]

Done.

+     *            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();
+            mUploadContentType = contentType;
+            mUploadChannel = channel;
+            mUploadContentLength = contentLength;
+            mUploadData = null;
+        }
+    }
+
+    public void setHttpMethod(String method) {
+        validateNotStarted();
+        if (!("PUT".equals(method) || "POST".equals(method))) {
+            throw new IllegalArgumentException("Only PUT or 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 &&
+                    ((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(mUrlRequestAdapter, method);
+            }
+
+            if (mHeaders != null && !mHeaders.isEmpty()) {
+                for (Entry<String, String> entry : mHeaders.entrySet()) {
+                    nativeAddHeader(mUrlRequestAdapter, entry.getKey(),
+                                    entry.getValue());
+              }

[mmenke, 2014/08/13 15:33:51]

indent +2

[mef, 2014/08/13 18:52:41]

Done.

+            }
+
+            if (mAdditionalHeaders != null) {
+                for (Entry<String, String> entry :
+                     mAdditionalHeaders.entrySet()) {
+                    nativeAddHeader(mUrlRequestAdapter, entry.getKey(),
+                                    entry.getValue());

[mmenke, 2014/08/13 15:33:50]

Is is really correct indentation?  It looks wrong.  :(

[mef, 2014/08/13 18:52:41]

Yeah, I'm not sure, I think continuation line is indented to the start of
parameters. I've changed it to 8 spaces.

+              }

[mmenke, 2014/08/13 15:33:50]

indent +2

[mef, 2014/08/13 18:52:41]

Done.

+            }
+
+            if (mUploadData != null && mUploadData.length > 0) {
+                nativeSetUploadData(mUrlRequestAdapter, mUploadContentType,
+                                    mUploadData);
+            } else if (mUploadChannel != null) {
+                nativeSetUploadChannel(mUrlRequestAdapter, mUploadContentType,
+                                       mUploadContentLength);
+            }
+
+            nativeStart(mUrlRequestAdapter);
+          }
+    }
+
+    public void cancel() {
+        synchronized (mLock) {
+            if (mCanceled) {
+                return;
+            }
+
+            mCanceled = true;
+
+            if (!mRecycled) {
+                nativeCancel(mUrlRequestAdapter);
+            }
+        }
+    }
+
+    public boolean isCanceled() {
+        synchronized (mLock) {
+            return mCanceled;
+        }
+    }
+
+    public boolean isRecycled() {
+        synchronized (mLock) {
+            return mRecycled;
+        }
+    }
+
+    public String getContentType() {
+        return mContentType;
+    }
+
+    public String getHeader(String name) {
+        validateHeadersAvailable();
+        return nativeGetHeader(mUrlRequestAdapter, name);
+    }
+
+    // All response headers.
+    public Map<String, List<String>> getAllHeaders() {
+        validateHeadersAvailable();
+        ResponseHeadersMap result = new ResponseHeadersMap();
+        nativeGetAllHeaders(mUrlRequestAdapter, result);
+        return result;
+    }
+
+    public String getUrl() {
+        return mUrl;
+    }
+
+    /**
+     * A callback invoked when the first chunk of the response has arrived.
+     */
+    @CalledByNative
     protected void onResponseStarted() {
-        super.onResponseStarted();
+        mContentType = nativeGetContentType(mUrlRequestAdapter);
+        mContentLength = nativeGetContentLength(mUrlRequestAdapter);
+        mHeadersAvailable = true;
 
-        mContentLength = super.getContentLength();
         if (mContentLengthLimit > 0 && mContentLength > mContentLengthLimit
                 && mCancelIfContentLengthOverLimit) {
             onContentLengthOverLimit();
@@ -103,7 +370,7 @@ public class ChromiumUrlRequest extends UrlRequest implements HttpUrlRequest {
 
         if (mOffset != 0) {
             // The server may ignore the request for a byte range.
-            if (super.getHttpStatusCode() == 200) {
+            if (getHttpStatusCode() == 200) {

[Charles, 2014/08/14 18:23:50]

This breaks range requests. Lower down in the file we're rewriting all 206 calls
to be 200 calls.

[mef, 2014/08/14 20:58:02]

Done.

[Charles, 2014/08/14 21:33:27]

This is still broken - the server will return 20
This isn't fixed, we're still rewriting the status codes. Range requests won't
work.

On 2014/08/14 20:58:02, mef wrote:

                 if (mContentLength != -1) {
                     mContentLength -= mOffset;

[mmenke, 2014/08/13 15:33:50]

This just seems bizarre...But fixing it is beyond the scope of this CL.

[mef, 2014/08/13 18:52:41]

Ack. Added TODO.

                 }
@@ -115,7 +382,14 @@ public class ChromiumUrlRequest extends UrlRequest implements HttpUrlRequest {
         mListener.onResponseStarted(this);
     }
 
-    @Override
+    /**
+     * Consumes a portion of the response.
+     *
+     * @param byteBuffer The ByteBuffer to append. Must be a direct buffer, and
+     *            no references to it may be retained after the method ends, as
+     *            it wraps code managed on the native heap.
+     */
+    @CalledByNative
     protected void onBytesRead(ByteBuffer buffer) {
         if (mContentLengthOverLimit) {
             return;
@@ -132,14 +406,40 @@ public class ChromiumUrlRequest extends UrlRequest implements HttpUrlRequest {
             }
         }
 
-        if (mContentLengthLimit != 0 && mSize > mContentLengthLimit) {
+        boolean contentLengthOverLimit =
+                (mContentLengthLimit != 0 && mSize > mContentLengthLimit);
+        if (contentLengthOverLimit) {
             buffer.limit(size - (int)(mSize - mContentLengthLimit));
-            super.onBytesRead(buffer);
+        }
+
+        try {
+            while (buffer.hasRemaining()) {
+                mSink.write(buffer);
+            }
+        } catch (IOException e) {
+            mSinkException = e;
+            cancel();
+        }
+        if (contentLengthOverLimit) {
             onContentLengthOverLimit();
-            return;
         }
+    }
 
-        super.onBytesRead(buffer);
+    private static int convertRequestPriority(int priority) {
+        switch (priority) {
+            case HttpUrlRequest.REQUEST_PRIORITY_IDLE:
+                return ChromiumUrlRequestPriority.IDLE;
+            case HttpUrlRequest.REQUEST_PRIORITY_LOWEST:
+                return ChromiumUrlRequestPriority.LOWEST;
+            case HttpUrlRequest.REQUEST_PRIORITY_LOW:
+                return ChromiumUrlRequestPriority.LOW;
+            case HttpUrlRequest.REQUEST_PRIORITY_MEDIUM:
+                return ChromiumUrlRequestPriority.MEDIUM;
+            case HttpUrlRequest.REQUEST_PRIORITY_HIGHEST:
+                return ChromiumUrlRequestPriority.HIGHEST;
+            default:
+                return ChromiumUrlRequestPriority.MEDIUM;
+        }
     }
 
     private void onContentLengthOverLimit() {
@@ -147,43 +447,136 @@ public class ChromiumUrlRequest extends UrlRequest implements HttpUrlRequest {
         cancel();
     }
 
-    @Override
+    /**
+     * A callback invoked when the response has been fully consumed.
+     */
     protected void onRequestComplete() {
         mListener.onRequestComplete(this);
     }
 
-    @Override
-    public int getHttpStatusCode() {
-        int httpStatusCode = super.getHttpStatusCode();
+    private void validateNotRecycled() {
+        if (mRecycled) {
+            throw new IllegalStateException("Accessing recycled request");
+        }
+    }
 
-        // TODO(mef): Investigate the following:
-        // If we have been able to successfully resume a previously interrupted
-        // download,
-        // the status code will be 206, not 200. Since the rest of the
-        // application is
-        // expecting 200 to indicate success, we need to fake it.
-        if (httpStatusCode == 206) {
-            httpStatusCode = 200;
+    private void validateNotStarted() {
+        if (mStarted) {
+            throw new IllegalStateException("Request already started");
         }
-        return httpStatusCode;
     }
 
-    @Override
-    public IOException getException() {
-        IOException ex = super.getException();
-        if (ex == null && mContentLengthOverLimit) {
-            ex = new ResponseTooLargeException();
+    private void validateHeadersAvailable() {
+        if (!mHeadersAvailable) {
+            throw new IllegalStateException("Response headers not available");
         }
-        return ex;
     }
 
-    @Override
-    public ByteBuffer getByteBuffer() {
-        return ((ChunkedWritableByteChannel)getSink()).getByteBuffer();
+    /**
+     * Notifies the listener, releases native data structures.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void finish() {
+        synchronized (mLock) {
+            mFinished = true;
+
+            if (mRecycled) {
+                return;
+            }
+            try {
+                mSink.close();
+            } catch (IOException e) {
+                // Ignore
+            }
+            onRequestComplete();
+            nativeDestroyRequestAdapter(mUrlRequestAdapter);
+            mUrlRequestAdapter = 0;
+            mRecycled = true;
+        }
     }
 
-    @Override
-    public byte[] getResponseAsBytes() {
-        return ((ChunkedWritableByteChannel)getSink()).getBytes();
+    /**
+     * Appends header |name| with value |value| to |headersMap|.
+     */
+    @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 a sequence of bytes from upload channel into the given buffer.
+     * @param dest The buffer into which bytes are to be transferred.
+     * @return Returns number of bytes read (could be 0) or -1 and closes
+     * the channel if error occured.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private int readFromUploadChannel(ByteBuffer dest) {
+        if (mUploadChannel == null || !mUploadChannel.isOpen())
+            return -1;
+        try {
+            int result = mUploadChannel.read(dest);
+            if (result < 0) {
+                mUploadChannel.close();
+                return 0;
+            }
+            return result;
+        } catch (IOException e) {
+            mSinkException = e;
+            try {
+                mUploadChannel.close();
+            } catch (IOException ignored) {
+                // Ignore this exception.
+            }
+            cancel();
+            return -1;
+        }
+    }
+
+    // Native methods are implemented in chromium_url_request.cc.
+
+    private native long nativeCreateRequestAdapter(
+            long ChromiumUrlRequestContextAdapter, String url, int priority);
+
+    private native void nativeAddHeader(long urlRequestAdapter, String name,
+            String value);
+
+    private native void nativeSetMethod(long urlRequestAdapter, String method);
+
+    private native void nativeSetUploadData(long urlRequestAdapter,
+            String contentType, byte[] content);
+
+    private native void nativeSetUploadChannel(long urlRequestAdapter,
+            String contentType, long contentLength);
+
+    private native void nativeStart(long urlRequestAdapter);
+
+    private native void nativeCancel(long urlRequestAdapter);
+
+    private native void nativeDestroyRequestAdapter(long urlRequestAdapter);
+
+    private native int nativeGetErrorCode(long urlRequestAdapter);
+
+    private native int nativeGetHttpStatusCode(long urlRequestAdapter);
+
+    private native String nativeGetErrorString(long urlRequestAdapter);
+
+    private native String nativeGetContentType(long urlRequestAdapter);
+
+    private native long nativeGetContentLength(long urlRequestAdapter);
+
+    private native String nativeGetHeader(long urlRequestAdapter, String name);
+
+    private native void nativeGetAllHeaders(long urlRequestAdapter,
+            ResponseHeadersMap headers);
+
+    // Explicit class to work around JNI-generator generics confusion.
+    private class ResponseHeadersMap extends HashMap<String, List<String>> {
     }
 }