Initial implementation of Cronet Async API.

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

Index: components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java
diff --git a/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java b/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java
index 3a7776dc8520cbabc219fde0c6515e7d1499e808..24241486156b6a9a2b8cf60e4c573c9d466fe601 100644
--- a/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java
+++ b/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java
@@ -4,33 +4,146 @@
 
 package org.chromium.net;
 
+import android.util.Log;
+
+import org.chromium.base.CalledByNative;
+import org.chromium.base.JNINamespace;
+
+import java.net.URL;
+import java.nio.ByteBuffer;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.Executor;
+
 /**
  * UrlRequest using Chromium HTTP stack implementation.
  */
-public class CronetUrlRequest implements UrlRequest {
+@JNINamespace("cronet")
+public final class CronetUrlRequest implements UrlRequest {

[Charles, 2014/09/23 22:33:12]

Should this class be package-private?

[mef, 2014/09/24 18:56:26]

Done.

+    /**
+     * Native adapter object, owned by UrlRequest.
+     */
+    private long mUrlRequestAdapter;
+    private final CronetUrlRequestContext mRequestContext;
+    private final String mUrl;
+    private final int mPriority;
+    private final UrlRequestListener mListener;
+    private final Executor mExecutor;
+    private NativeResponseInfo mResponseInfo;
+    private boolean mCanceled = false;
+    private OnDataReceivedRunnable mOnDataReceivedTask;
+
+    final class OnDataReceivedRunnable implements Runnable {

[Charles, 2014/09/23 22:33:12]

I see how this could be trivially extended to work with Channels, but is there a
reason we didn't just use WritableByteChannel for something that only exists to
write bytes? Will we add API to do that?

[mef, 2014/09/24 18:56:26]

I think we could, but should we? What was the consensus on net-dev@ discussion?

[mmenke, 2014/09/24 19:04:14]

WriteableByteChannel adds a copy, doesn't it?  It's also a blocking API.

I'm not really sure just what you have in mind here - do we use one internally
for our own purposes, or are you thinking having the embedder give us one?  Then
we'd...what, tell the embedder when bytes are available, and how many, then
write to those bytes to the channel, and hope the implementation is such that if
you write n bytes, the reader will always be able to reach all n bytes, without
some sort of hidden buffering without a timeout?

That seems like a much uglier API.

[mef, 2014/09/26 19:23:52]

Per chat with clm@ WriteableByteChannel is just an interface, and its
'write(ByteBuffer src)' method seems to closely match onDataReceived callback.
We did agree though that it could be easily done through simple listener
wrapper.

[Charles, 2014/09/26 22:52:26]

What? No. I'm just remarking that the use case of onDataReceived is remarkably
similar to that of WriteableByteChannel.write(ByteBuffer b). Yes, it's blocking,
but that's OK because we're running on the executor that's off the network
thread. 

+        ByteBuffer mByteBuffer;
+        public void run() {
+            try {
+                mListener.onDataReceived(CronetUrlRequest.this, mByteBuffer);
+                if (!isCanceled())
+                    nativeReceiveData(mUrlRequestAdapter);
+            } catch (Exception e) {
+                onCalledByNativeException(e);
+            }
+        }
+    }
+
+    final class NativeResponseInfo implements ResponseInfo {

[Charles, 2014/09/23 22:33:12]

Can we make these fields final and populate them all in the constructor?

[mef, 2014/09/24 18:56:26]

Done.

+        private final List<String> mUrlChain = new ArrayList<String>();
+        private int mHttpStatusCode = 0;
+        private final ResponseHeadersMap mAllHeaders = new ResponseHeadersMap();
+        private boolean mWasCached = false;
+        private String mNegotiatedProtocol = "";
+
+        @Override
+        public String getUrl() {
+            return mUrlChain.get(mUrlChain.size() - 1);
+        }
+
+        @Override
+        public String[] getUrlChain() {
+            return mUrlChain.toArray(new String[mUrlChain.size()]);
+        }
+
+        @Override
+        public int getHttpStatusCode() {
+            return mHttpStatusCode;
+        }
+
+        @Override
+        public Map<String, List<String>> getAllHeaders() {
+            return mAllHeaders;
+        }
+
+        @Override
+        public boolean wasCached() {
+            return mWasCached;
+        }
+
+        @Override
+        public String getNegotiatedProtocol() {
+            return mNegotiatedProtocol;
+        }
+    };
+
+    CronetUrlRequest(CronetUrlRequestContext requestContext,
+            String url, int priority,
+            UrlRequestListener listener,
+            Executor executor) {
+        if (requestContext == null) {
+            throw new NullPointerException("Context is required");
+        }
+        if (url == null) {
+            throw new NullPointerException("URL is required");
+        }
+        if (listener == null) {
+            throw new NullPointerException("Listener is required");
+        }
+        if (executor == null) {
+            throw new NullPointerException("Executor is required");
+        }
+
+        mRequestContext = requestContext;
+        mUrl = url;
+        mPriority = convertRequestPriority(priority);
+        mUrlRequestAdapter = nativeCreateRequestAdapter(
+                mRequestContext.getCronetUrlRequestContextAdapter(),
+                mUrl,
+                mPriority);
+        mListener = listener;
+        mExecutor = executor;
+    }
+
     @Override
     public void setHttpMethod(String method) {
-
+        if (method == null) {
+            throw new NullPointerException("method is required");
+        }
+        nativeSetHttpMethod(mUrlRequestAdapter, method);
     }
 
     @Override
     public void addHeader(String header, String value) {
-
+        if (header == null || value == null) {
+            throw new NullPointerException("Invalid header");
+        }
+        nativeAddHeader(mUrlRequestAdapter, header, value);
     }
 
     @Override
-    public void start(UrlRequestListener listener) {
-
+    public void start() {
+        nativeStart(mUrlRequestAdapter);
     }
 
     @Override
     public void cancel() {
-
+        mCanceled = true;
+        nativeCancel(mUrlRequestAdapter);
     }
 
     @Override
     public boolean isCanceled() {

[Charles, 2014/09/23 22:33:12]

Thread safe? Sometimes we check mCanceled, sometimes we use the method. We
should be consistent.

[mef, 2014/09/24 18:56:26]

Hmm, I don't see where we check mCanceled. I've removed mCanceled = true; from
cancel, so it is only set in OnComplete.

-        return false;
+        return mCanceled;
     }
 
     @Override
@@ -47,4 +160,229 @@ public class CronetUrlRequest implements UrlRequest {
     public void resume() {

[Charles, 2014/09/23 22:33:12]

throw new UnsupportedOperationException("Not implemented yet")

[mef, 2014/09/24 18:56:26]

Done.

 
     }
+
+    private static int convertRequestPriority(int priority) {
+        switch (priority) {
+            case REQUEST_PRIORITY_IDLE:
+                return ChromiumUrlRequestPriority.IDLE;
+            case REQUEST_PRIORITY_LOWEST:
+                return ChromiumUrlRequestPriority.LOWEST;
+            case REQUEST_PRIORITY_LOW:
+                return ChromiumUrlRequestPriority.LOW;
+            case REQUEST_PRIORITY_MEDIUM:
+                return ChromiumUrlRequestPriority.MEDIUM;
+            case REQUEST_PRIORITY_HIGHEST:
+                return ChromiumUrlRequestPriority.HIGHEST;
+            default:
+                return ChromiumUrlRequestPriority.MEDIUM;
+        }
+    }
+
+    private void prepareResponseInfo() {
+        if (mResponseInfo != null)
+            return;
+        mResponseInfo = new NativeResponseInfo();
+        mResponseInfo.mUrlChain.add(mUrl);
+        mResponseInfo.mHttpStatusCode = nativeGetHttpStatusCode(
+                mUrlRequestAdapter);
+        nativeGetAllHeaders(mUrlRequestAdapter, mResponseInfo.mAllHeaders);
+        mResponseInfo.mWasCached = false;
+        mResponseInfo.mNegotiatedProtocol = nativeGetNegotiatedProtocol(
+                mUrlRequestAdapter);
+    }
+
+    // Private methods called by native library.
+
+    /**
+     * If @CalledByNative method throws an exception, request gets cancelled
+     * and exception could be retrieved from request using getException().
+     */
+    private void onCalledByNativeException(Exception e) {
+        UrlRequestException requestError = new UrlRequestException(
+                "CalledByNative method has thrown an exception", e);
+        Log.e(CronetUrlRequestContext.LOG_TAG,
+                "Exception in CalledByNative method", e);
+        try {
+            mListener.onError(this, requestError);
+            cancel();
+        } catch (Exception cancelException) {
+            Log.e(CronetUrlRequestContext.LOG_TAG,
+                    "Exception trying to cancel request", cancelException);
+        }
+    }
+
+    /**
+     * Called before following redirects.  The redirect will automatically be
+     * followed, unless the request is paused or cancelled during this
+     * callback.  If the redirect response has a body, it will be ignored.
+     * This will only be called between start and onResponseStarted.
+     *
+     * @param info Response information.
+     * @param newLocation Location where request is redirected.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void onRedirect(final String newLocation) {
+        prepareResponseInfo();
+        mResponseInfo.mUrlChain.add(0, newLocation);

[xunjieli, 2014/09/23 20:53:24]

It looks like your mUrlChain will append the original url to the end, while add
the redirects to the front of the array. This is different from your docs on
getUrlChain(). Is that intended? I might have missed something..

[mef, 2014/09/24 18:56:26]

Done. Good catch, thanks!

+        Runnable task = new Runnable() {
+            public void run() {
+                try {
+                    mListener.onRedirect(CronetUrlRequest.this, mResponseInfo,
+                        new URL(newLocation));
+                    if (!isCanceled())
+                        nativeFollowDeferredRedirect(mUrlRequestAdapter);
+                } catch (Exception e) {
+                    onCalledByNativeException(e);
+                }
+            }
+        };
+        mExecutor.execute(task);
+    }
+
+    /**
+     * Called when the final set of headers, after all redirects,
+     * is received. Can only be called once for each request.
+     *
+     * @param info Response information.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void onResponseStarted() {
+        prepareResponseInfo();
+        Runnable task = new Runnable() {
+            public void run() {
+                try {
+                    mListener.onResponseStarted(CronetUrlRequest.this,
+                                                mResponseInfo);
+                    if (!isCanceled())
+                        nativeReceiveData(mUrlRequestAdapter);
+                } catch (Exception e) {
+                    onCalledByNativeException(e);
+                }
+            }
+        };
+        mExecutor.execute(task);
+    }
+
+    /**
+     * Called whenever data is received.  The ByteBuffer remains
+     * valid only for the duration of the callback.  Or if the callback

[Charles, 2014/09/23 22:33:12]

This isn't true any more, the bytebuffer should be valid until the received
method is called.

[mef, 2014/09/24 18:56:26]

Done.

+     * pauses the request, it remains valid until the request is resumed.
+     * Cancelling the request also invalidates the buffer.
+     *
+     * @param byteBuffer Received data.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void onDataReceived(final ByteBuffer byteBuffer) {
+        if (mOnDataReceivedTask == null)
+            mOnDataReceivedTask = new OnDataReceivedRunnable();
+        mOnDataReceivedTask.mByteBuffer = byteBuffer;
+        mExecutor.execute(mOnDataReceivedTask);
+    }
+
+    /**
+     * Called when request is complete, no callbacks will be called afterwards.

[Charles, 2014/09/23 22:33:12]

We should propagate all this javadoc onto the listener, and update the javadoc
here.

[mef, 2014/09/24 18:56:26]

The javadoc here actually came from the listener.

+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void onComplete(boolean canceled) {
+        mCanceled = canceled;
+        Runnable task = new Runnable() {
+            public void run() {
+                try {
+                    mListener.onComplete(CronetUrlRequest.this);

[Charles, 2014/09/23 22:33:12]

You're giving the listener a reference to the request, but there's aren't any
methods in the UrlRequest interface that are useful for the listener to call
during onComplete.

[mmenke, 2014/09/23 22:38:22]

This allows one listener to handle multiple requests.  I don't think it will be
the usual use case, but doesn't seem to be much cost to making an interface that
allows it.

[Charles, 2014/09/23 22:50:51]

Exposing the request object allows a listener to bring it out of scope, which is
a problem since the object is no longer valid after the call completes. I don't
think having a single listener for multiple requests is all that important, and
you can do that anyway if the URLs are unique by passing a result object.

[mef, 2014/09/24 18:56:26]

Well, listener could bring UrlRequest out of scope by storing it from any other
callback as well, so IMHO passing it explicitly to all callbacks doesn't hurt. I
don't see how URLs could be expected to be unique. 
It could make sense to make ResponseInfo available after request completion, so
for example, caller could check http status, etc.

+                    nativeDestroyRequestAdapter(mUrlRequestAdapter);
+                } catch (Exception e) {
+                    onCalledByNativeException(e);
+                }
+            }
+        };
+        mExecutor.execute(task);
+    }
+
+    /**
+     * Called when error has occured, no callbacks will be called afterwards.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void onError(int error) {
+        Runnable task = new Runnable() {
+            public void run() {
+                try {
+                    UrlRequestException requestError = new UrlRequestException(
+                            "Exception in CronetUrlRequest ", null);
+                        mListener.onError(CronetUrlRequest.this, requestError);
+                    nativeDestroyRequestAdapter(mUrlRequestAdapter);
+                } catch (Exception e) {
+                    onCalledByNativeException(e);
+                }
+            }
+        };
+        mExecutor.execute(task);
+    }
+
+    /**
+     * Appends header |name| with value |value| to |headersMap|.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void onAppendResponseHeader(ResponseHeadersMap headersMap,
+            String name, String value) {
+        try {
+            if (!headersMap.containsKey(name)) {
+                headersMap.put(name, new ArrayList<String>());
+            }
+            headersMap.get(name).add(value);
+        } catch (Exception e) {
+            onCalledByNativeException(e);
+        }
+    }
+
+    // Native methods are implemented in cronet_url_request.cc.
+
+    private native long nativeCreateRequestAdapter(
+            long urlRequestContextAdapter, String url, int priority);
+
+    private native void nativeAddHeader(long urlRequestAdapter, String name,
+            String value);
+
+    private native void nativeSetHttpMethod(long urlRequestAdapter,
+            String method);
+
+    private native void nativeStart(long urlRequestAdapter);
+
+    private native void nativeCancel(long urlRequestAdapter);
+
+    private native void nativeDestroyRequestAdapter(long urlRequestAdapter);
+
+    private native void nativeFollowDeferredRedirect(long urlRequestAdapter);
+
+    private native void nativeReceiveData(long urlRequestAdapter);
+
+    // TODO(mef): Remove unused methods (if any).
+
+    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);
+
+    private native String nativeGetNegotiatedProtocol(long urlRequestAdapter);
+
+    // Explicit class to work around JNI-generator generics confusion.
+    private class ResponseHeadersMap extends HashMap<String, List<String>> {
+    }
+
+
 }