[Cronet] Implement getOutputStream in CronetHttpURLConnection

components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java

Index: components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
diff --git a/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java b/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
index 585b6e6af7c75855eac1b29dae12a5e015f4fae0..787e4bf4fafad08d0169e8b5c4b3914a71ff4a1d 100644
--- a/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
+++ b/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
@@ -4,10 +4,12 @@
 
 package org.chromium.net.urlconnection;
 
+import android.util.Log;
 import android.util.Pair;
 
 import org.chromium.net.ExtendedResponseInfo;
 import org.chromium.net.ResponseInfo;
+import org.chromium.net.UploadDataProvider;
 import org.chromium.net.UrlRequest;
 import org.chromium.net.UrlRequestContext;
 import org.chromium.net.UrlRequestException;
@@ -16,8 +18,10 @@ import org.chromium.net.UrlRequestListener;
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.OutputStream;
 import java.net.HttpURLConnection;
 import java.net.MalformedURLException;
+import java.net.ProtocolException;
 import java.net.URL;
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
@@ -33,18 +37,23 @@ import java.util.TreeMap;
  * attempted.
  */
 public class CronetHttpURLConnection extends HttpURLConnection {
+    private static final String TAG = "CronetHttpURLConnection";
     private final UrlRequestContext mUrlRequestContext;
     private final MessageLoop mMessageLoop;
     private final UrlRequest mRequest;
     private final List<Pair<String, String>> mRequestHeaders;
 
     private CronetInputStream mInputStream;
+    private OutputStream mOutputStream;
     private ResponseInfo mResponseInfo;
     private UrlRequestException mException;
     private ByteBuffer mResponseByteBuffer;
     private boolean mOnRedirectCalled = false;
+    // Use a member variable for the long version of fixed content length, since
+    // super.fixedContentLengthLong is only added in API 19.
+    private long mFixedContentLength = -1;
 
-    protected CronetHttpURLConnection(URL url,
+    public CronetHttpURLConnection(URL url,
             UrlRequestContext urlRequestContext) {
         super(url);
         mUrlRequestContext = urlRequestContext;
@@ -63,21 +72,7 @@ public class CronetHttpURLConnection extends HttpURLConnection {
      */
     @Override
     public void connect() throws IOException {
-        if (connected) {
-            checkHasResponse();
-            return;
-        }
-        connected = true;
-        for (Pair<String, String> requestHeader : mRequestHeaders) {
-            mRequest.addHeader(requestHeader.first, requestHeader.second);
-        }
-        if (!getUseCaches()) {
-            mRequest.disableCache();
-        }
-        mRequest.start();
-        // Blocks until onResponseStarted or onFailed is called.
-        mMessageLoop.loop();
-        checkHasResponse();
+        startRequest(true);
     }
 
     /**
@@ -198,6 +193,73 @@ public class CronetHttpURLConnection extends HttpURLConnection {
         return mInputStream;
     }
 
+    @Override
+    public OutputStream getOutputStream() throws IOException {
+        if (mOutputStream == null) {
+            if (connected) {
+                throw new ProtocolException(
+                        "Cannot write to OutputStream after receiving response.");
+            }
+            if (mFixedContentLength != -1) {
+                addRequestProperty("Content-Length",
+                        Long.toString(mFixedContentLength));
+                mOutputStream = new CronetOutputStream(this, mFixedContentLength);
+                // Start the request now since all headers can be sent.
+                startRequest(false);
+            } else {
+                // For the buffered case, start the request only when
+                // content-length bytes are received, or when an
+                // connect action is initiated by the consumer.
+                Log.d(TAG, "Outputstream is being buffered in memory.");
+                String length = getRequestProperty("Content-Length");
+                if (length == null) {
+                    mOutputStream = new CronetBufferedOutputStream(this, -1);
+                } else {
+                    long lengthParsed = Long.parseLong(length);
+                    mOutputStream = new CronetBufferedOutputStream(this, lengthParsed);
+                }
+            }
+        }
+        return mOutputStream;
+    }
+
+    /**
+     * Starts the request if {@code connected} is false. If {@code readResponse}
+     * is true, block until the response is received.
+     */
+    private void startRequest(boolean readResponse) throws IOException {
+        if (connected) {
+            if (readResponse) {
+                checkHasResponse();
+            }
+            return;
+        }
+        if (mOutputStream != null) {
+            // Default Content-Type to application/x-www-form-urlencoded
+            if (getRequestProperty("Content-Type") == null) {
+                addRequestProperty("Content-Type",
+                        "application/x-www-form-urlencoded");
+            }
+            mRequest.setUploadDataProvider((UploadDataProvider) mOutputStream,
+                    mMessageLoop);
+        }
+        connected = true;
+        // Start the request. Note that connect() is not used since

[pauljensen, 2015/03/10 17:31:05]

Can we move this comment down so it's right before mRequest.start()?  I think it
makes more sense there.

[xunjieli, 2015/03/12 21:55:09]

Done.

+        // connect() blocks until headers are received.

[pauljensen, 2015/03/10 17:31:05]

I think it's a little odd to mention using connect() in a function that
implements connect().  Part of me wonders if we should remove the readResponse
logic and change connect() to:
  if (connected) checkHasResponse();
  startRequest();
  // Blocks until onResponseStarted or onFailed is called.
  mMessageLoop.loop();
  checkHasResponse();
or perhaps even simpler:
  if (!connected) {
    startRequest();
    // Blocks until onResponseStarted or onFailed is called.
    mMessageLoop.loop();
  }
  checkHasResponse();

[xunjieli, 2015/03/12 21:55:09]

Done. Good idea! I was thinking connect() shouldn't block when I did the
readResponse logic, but ended up not finding a way around it. Thanks!

+        for (Pair<String, String> requestHeader : mRequestHeaders) {
+            mRequest.addHeader(requestHeader.first, requestHeader.second);
+        }
+        if (!getUseCaches()) {
+            mRequest.disableCache();
+        }
+        mRequest.start();
+        if (readResponse) {
+            // Blocks until onResponseStarted or onFailed is called.
+            mMessageLoop.loop();
+            checkHasResponse();
+        }
+    }
+
     /**
      * Returns an input stream from the server in the case of an error such as
      * the requested file has not been found on the remote server.
@@ -301,6 +363,35 @@ public class CronetHttpURLConnection extends HttpURLConnection {
     }
 
     /**
+     * Sets chunked streaming mode.
+     */
+    @Override
+    public void setChunkedStreamingMode(int chunklen) {
+        // TODO(xunjieli): implement this.
+        throw new UnsupportedOperationException("Chunked mode not supported yet");
+    }
+
+    @Override
+    public void setFixedLengthStreamingMode(int contentLength) {
+        setFixedLengthStreamingMode((long) contentLength);
+    }
+
+    @Override

[pauljensen, 2015/03/10 17:31:05]

Can we be overriding this if it is not present on prior API versions?

[xunjieli, 2015/03/12 21:55:09]

There's no warning, so I guess it's not prohibited. I have adopted your
reflection suggestion.

+    public void setFixedLengthStreamingMode(long contentLength) {
+        if (super.connected) {
+            throw new IllegalStateException("Already connected");
+        }
+        if (super.chunkLength > 0) {
+            throw new IllegalStateException("Already in chunked mode");
+        }
+        if (contentLength < 0) {
+            throw new IllegalArgumentException("contentLength < 0");
+        }
+        mFixedContentLength = contentLength;
+        super.fixedContentLength = (int) Math.min(contentLength, Integer.MAX_VALUE);

[pauljensen, 2015/03/10 17:31:05]

I still feel like this should call super.setFixedLengthStreamingMode(),
otherwise the super's long version won't be updated properly.  Part of me
wonders if we should be overriding this at all, and instead replace uses of
mFixedContentLength with accesses to super.fixedContentLength?  Perhaps we'll
have to use some reflection to see if the parent has the long field.

[xunjieli, 2015/03/12 21:55:09]

Done. Thanks for the suggestion!

+    }
+
+    /**
      * Used by {@link CronetInputStream} to get more data from the network
      * stack. This should only be called after the request has started. Note
      * that this call might block if there isn't any more data to be read.
@@ -312,6 +403,21 @@ public class CronetHttpURLConnection extends HttpURLConnection {
     }
 
     /**
+     * Used by {@link CronetOutputStream} to wait for data consumed by the
+     * network stack before letting the consumer write more data.
+     */
+    void waitForPostDataConsumed() throws IOException {
+        mMessageLoop.loop();
+    }
+
+    /**
+     * Used by {@link CronetOutputStream} to wait for consumer to write more data.
+     */
+    void waitForPostData() {
+        mMessageLoop.postQuitTask();
+    }
+
+    /**
      * Returns the index of request header in {@link #mRequestHeaders} or
      * -1 if not found.
      */