[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..2e15151f3ce28f43024bb4b10afde91549dd6f64 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,20 @@ 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;
 
-    protected CronetHttpURLConnection(URL url,
+    public CronetHttpURLConnection(URL url,
             UrlRequestContext urlRequestContext) {
         super(url);
         mUrlRequestContext = urlRequestContext;
@@ -63,20 +69,11 @@ 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();
+        if (!connected) {
+            startRequest();
+            // Blocks until onResponseStarted or onFailed is called.
+            mMessageLoop.loop();
         }
-        mRequest.start();
-        // Blocks until onResponseStarted or onFailed is called.
-        mMessageLoop.loop();
         checkHasResponse();
     }
 
@@ -198,6 +195,80 @@ 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.");
+            }
+            // Use reflection to see whether fixedContentLengthLong (only added
+            // in API 19) is inherited.

[mmenke, 2015/03/13 14:51:12]

I don't suppose we can just make our own implementation of
setFixedLengthStreamingMode (long and short versions)?  Admittedly, we couldn't
mark the second one with @Override, it wouldn't be on older API versions, but it
would allow us to test the functionality, regardless of API version.

I don't know Java well enough to know if it would complain about having one
overloaded member act as an override, and the other not.

[xunjieli, 2015/03/13 19:26:40]

I adopted this approach earlier, but changed after Paul suggested reflection,
which I think is slightly cleaner. FYI, okhttp does use the
override-with-own-implementation approach
(https://github.com/square/okhttp/blob/master/okhttp-urlconnection/src/main/ja...)
It won't complain. But it looks like "@Override" is removed in aosp
http://androidxref.com/4.4.3_r1.1/xref/external/okhttp/src/main/java/com/squa....

+            long superFixedContentLengthLong = -1;
+            try {
+                Class parent = this.getClass();
+                superFixedContentLengthLong =
+                        (long) (parent.getField("fixedContentLengthLong").get(this));

[mmenke, 2015/03/13 14:51:11]

Instead of the cast, there's a getLong method.

[xunjieli, 2015/03/13 19:26:40]

Done.

+            } catch (Exception e) {
+                // Ignored.
+            }
+            if (fixedContentLength != -1) {
+                mOutputStream = new CronetOutputStream(this, fixedContentLength);
+                // Start the request now since all headers can be sent.
+                startRequest();
+            } else if (superFixedContentLengthLong != -1) {
+                mOutputStream = new CronetOutputStream(this, superFixedContentLengthLong);
+                // Start the request now since all headers can be sent.
+                startRequest();

[mmenke, 2015/03/13 14:51:11]

This seems overly complicated.

Why not:

long contentLength = fixedContentLength;
// Try to get superFixedContentLengthLong here, and overwrite contentLength with
it.
// API docs are unclear if we need to check if superFixedContentLengthLong is -1
// before overwrite or not.
if (contentLength != -1) {
  ...
}

[xunjieli, 2015/03/13 19:26:40]

Done. Added check before overriding. Looking at the official implementation, the
int version and the long version are completely separated. If the user only sets
the int version, long version is -1, so we wouldn't want to override.

+            } 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);
+                } else {
+                    long lengthParsed = Long.parseLong(length);
+                    mOutputStream = new CronetBufferedOutputStream(this, lengthParsed);

[mmenke, 2015/03/13 14:51:11]

Should we do something if we can't parse the length as a long, or it's < 0?

[xunjieli, 2015/03/13 19:26:40]

If there's a parse error, an runtime exception (NumberFormatException) will be
propagated to the user. If it < 0, the CronetBufferedOutputStream constructor
will complain.
I am not sure if we can do anything else. Maybe we can catch
NumberFormatException and IllegalArgumentException and make it into an
IOException? Not sure if that's a good idea. 

[mmenke, 2015/03/25 18:34:57]

As long as things will fail, I'm fine with the current code.

[xunjieli, 2015/03/27 17:46:44]

Acknowledged.

+                }
+            }
+        }
+        return mOutputStream;
+    }
+
+    /**
+     * Starts the request if {@code connected} is false.
+     */
+    private void startRequest() throws IOException {
+        if (connected) {
+            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");
+            }
+            if (getRequestProperty("Content-Length") == null) {
+                addRequestProperty("Content-Length",
+                        Long.toString(((UploadDataProvider) mOutputStream).getLength()));
+            }
+            mRequest.setUploadDataProvider((UploadDataProvider) mOutputStream,
+                    mMessageLoop);
+        }
+        connected = true;
+        // Start the request. Note that connect() is not used since
+        // connect() blocks until headers are received.
+        for (Pair<String, String> requestHeader : mRequestHeaders) {
+            mRequest.addHeader(requestHeader.first, requestHeader.second);
+        }
+        if (!getUseCaches()) {
+            mRequest.disableCache();
+        }
+        mRequest.start();
+    }
+
     /**
      * 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 +372,15 @@ 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");
+    }
+
+    /**
      * 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 +392,23 @@ public class CronetHttpURLConnection extends HttpURLConnection {
     }
 
     /**
+     * Used by {@link CronetOutputStream} to wait for data to be read by the
+     * network stack before letting the consumer write more data.
+     */
+    void waitForRead() throws IOException {
+        mMessageLoop.loop();
+    }
+
+    /**
+     * Used by {@link CronetOutputStream} to execute read after consumer writes
+     * more data.
+     */
+    void postponeRead(Runnable readTask) {
+        mMessageLoop.postQuitTask();
+        mMessageLoop.execute(readTask);
+    }
+
+    /**
      * Returns the index of request header in {@link #mRequestHeaders} or
      * -1 if not found.
      */