[Cronet] Initial implementation of HttpURLConnection

components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.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.urlconnection;
+
+import org.chromium.net.ExtendedResponseInfo;
+import org.chromium.net.ResponseInfo;
+import org.chromium.net.UrlRequest;
+import org.chromium.net.UrlRequestContext;
+import org.chromium.net.UrlRequestException;
+import org.chromium.net.UrlRequestListener;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.HttpURLConnection;
+import java.net.URL;
+import java.nio.ByteBuffer;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.Executor;
+import java.util.concurrent.LinkedBlockingQueue;
+
+/**
+ * An implementation of HttpURLConnection that uses Cronet to send requests and
+ * receive response. This class inherits a {@code connected} field from the
+ * superclass. That field indicates whether a connection has ever been
+ * attempted.
+ */
+public class CronetHttpURLConnection extends HttpURLConnection {
+    private final UrlRequestContext mUrlRequestContext;
+    private final MessageLoop mMessageLoop;
+    private final BlockingQueue<MessageLoop.Message> mMessageQueue;
+
+    private UrlRequest mRequest;
+    private ResponseInfo mResponseInfo;
+    private ByteBufferInputStream mResponseBody;

[mmenke, 2014/11/20 18:52:37]

Name here is confusing.  Maybe just mInputStream?

[xunjieli, 2014/11/20 20:32:00]

Done.

+    private ByteBuffer mResponseByteBuffer;
+
+    protected CronetHttpURLConnection(URL url,
+            UrlRequestContext urlRequestContext) {
+        super(url);
+        mUrlRequestContext = urlRequestContext;
+        mMessageQueue = new LinkedBlockingQueue<MessageLoop.Message>();
+        mMessageLoop = new MessageLoop(mMessageQueue);
+    }
+
+    /**
+     * Wrapper executor class which posts tasks to current looper.
+     */
+    private class HandlerThreadExecutor implements Executor {
+        public HandlerThreadExecutor() {
+        }
+
+        @Override
+        public void execute(Runnable command) {
+            try {
+                mMessageQueue.put(new MessageLoop.Message(command));
+            } catch (InterruptedException e) {
+                e.printStackTrace();
+            }
+        }
+    }
+
+    /**
+     * Opens a connection to the resource.
+     */
+    @Override
+    public void connect() throws IOException {

[mmenke, 2014/11/20 18:52:37]

Question:  With the default implementation, I assume connect() can't be used
more than once on the same object, to re-request a resource?

[xunjieli, 2014/11/20 20:31:59]

Right, the documentation says if connect() is called a second time, the call is
ignored
(http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/...).

In maybeStartRequest(), I checked whether connected is true. 

+        maybeStartRequest();
+    }
+
+    /**
+     * Releases this connection so that its resources may be either reused or
+     * closed.
+     */
+    @Override
+    public void disconnect() {
+        if (mResponseBody != null) {
+            try {
+                mResponseBody.close();
+            } catch (IOException e) {
+                e.printStackTrace();
+            }
+        }
+    }
+
+    /**
+     * Returns the response message returned by the remote HTTP server.
+     */
+    @Override
+    public String getResponseMessage() {
+        maybeStartRequest();
+        return mResponseInfo.getHttpStatusText();
+    }
+
+    /**
+     * Returns the response code returned by the remote HTTP server.
+     */
+    @Override
+    public int getResponseCode() {
+        maybeStartRequest();
+        return mResponseInfo.getHttpStatusCode();
+    }
+
+    /**
+     * Returns an InputStream for reading data from the resource pointed by this
+     * URLConnection.
+     */
+    @Override
+    public InputStream getInputStream() {
+        maybeStartRequest();

[mmenke, 2014/11/20 18:52:37]

optional nit:  Think this makes more sense in the body of the if.

[xunjieli, 2014/11/20 20:31:59]

Done.

+        if (mResponseBody == null) {
+            mResponseBody = new ByteBufferInputStream(mMessageLoop);
+        }
+        return mResponseBody;
+    }
+
+    /**
+     * Adds the given property to the request header.
+     */
+    @Override
+    public final void addRequestProperty(String key, String value) {
+        setRequestProperty(key, value);
+    }
+
+    /**
+     * Sets the value of the specified request header field.
+     */
+    @Override
+    public final void setRequestProperty(String key, String value) {
+        if (connected) {
+            throw new IllegalStateException(
+                    "Cannot set request property after connection is made");
+        }
+        maybeCreateRequest();
+        mRequest.addHeader(key, value);
+    }
+
+    /**
+     * Returns whether this connection uses a proxy server or not.
+     */
+    @Override
+    public boolean usingProxy() {
+        // TODO(xunjieli): implement this.
+        return false;
+    }
+
+    private class CronetUrlRequestListener implements UrlRequestListener {
+

[mmenke, 2014/11/20 18:52:37]

nit:  Remove blank line.

[xunjieli, 2014/11/20 20:31:59]

Done.

+        public CronetUrlRequestListener() {
+        }
+
+        @Override
+        public void onResponseStarted(UrlRequest request, ResponseInfo info) {
+            mResponseInfo = info;
+            // Quits the message loop since we have the headers now.
+            mMessageLoop.quit();
+        }
+
+        @Override
+        public void onDataReceived(UrlRequest request, ResponseInfo info,
+                ByteBuffer byteBuffer) {
+            mResponseInfo = info;
+            mResponseByteBuffer =

[mmenke, 2014/11/20 18:52:37]

I don't think we currently need a member variable for this.  Alternatively, we
could have the ByteBufferInputStream call into |this| both to get the byte
buffer, and to access the current state of the connection, rather than using a
push-based model.  May make the code a little simpler.

[xunjieli, 2014/11/20 20:32:00]

Done. Changed to pull-based model, so we have one fewer method. Thanks for the
suggestion

+                    ByteBuffer.allocateDirect(byteBuffer.capacity());

[mmenke, 2014/11/20 18:52:37]

Should we be using allocate here instead?

[xunjieli, 2014/11/20 20:31:59]

Done. Not exactly sure which one to use. The original byte buffer is a direct
bytebuffer, so I did allocateDirect(). But I guess this bytebuffer copy is on
java side the whole time, so allocate() should be equivalent.

+            mResponseByteBuffer.put(byteBuffer);
+            mResponseByteBuffer.rewind();
+            mResponseBody.setByteBuffer(mResponseByteBuffer);
+            mMessageLoop.quit();
+        }
+
+        @Override
+        public void onRedirect(UrlRequest request, ResponseInfo info,
+                String newLocationUrl) {
+            // TODO(xunjieli): Handle redirect.
+            mResponseInfo = info;
+        }
+
+        @Override
+        public void onSucceeded(UrlRequest request, ExtendedResponseInfo info) {
+            setResponseDataCompleted();
+        }
+
+        @Override
+        public void onFailed(UrlRequest request, ResponseInfo info,
+                UrlRequestException exception) {
+            // TODO(xunjieli): Handle failure.
+            setResponseDataCompleted();

[mmenke, 2014/11/20 18:52:36]

I think it's weird that onSucceeded/onFailed have the outer class call into the
buffer and exit the message loop, while onDataReceived does those itself.  I'm
fine with either the inner our outer class do all the work, just think we should
be consistent.

[xunjieli, 2014/11/20 20:32:00]

Done. Havent noticed that. thanks!

+        }
+    }
+
+    /**
+     * On first call, creates and starts UrlRequest, and waits until response
+     * headers are received. Does nothing on subsequent calls.
+     */
+    private void maybeStartRequest() {
+        if (connected) {
+            return;
+        }
+        maybeCreateRequest();
+        mRequest.start();
+        connected = true;

[mmenke, 2014/11/20 18:52:37]

Does it make sense to do this even in the case we fail to establish a
connection?  The docs aren't clear on this topic.  Know you aren't currently
handling errors, anyways.

[xunjieli, 2014/11/20 20:31:59]

Done. I think it makes sense to set connected to true no matter whether the
request succeeds or fails. I moved the assignment to before mRequest.start().

+        // Blocks until onResponseStarted or onFailed is called.
+        mMessageLoop.loop();
+    }
+
+    /**
+     * Maybe creates {@code mRequest} if it is null.
+     */
+    private void maybeCreateRequest() {

[mmenke, 2014/11/20 18:52:37]

Can we just create this in the constructor, rather than doing lazy
initialization?  Don't think we get enough out of the lazy initialization to be
worth the complexity.  This adds the risk that any feature that requires the
request exist before we use it will result in a regression.

[xunjieli, 2014/11/20 20:32:00]

Done. Good idea! thanks.

+        if (mRequest != null) {
+            return;
+        }
+        mRequest = mUrlRequestContext.createRequest(url.toString(),
+                new CronetUrlRequestListener(), new HandlerThreadExecutor());
+    }
+
+    /**
+     * Notifies {@link #mResponseBody} that transferring of response data has
+     * completed.
+     */
+    private void setResponseDataCompleted() {
+        if (mResponseBody != null) {
+            mResponseBody.setResponseDataCompleted();
+        }
+        mMessageLoop.quit();
+    }
+}