[Cronet] Fix CronetFixedModeOutputStream to not write more bytes than specified

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

Index: components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java
diff --git a/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java b/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java
index f6e005335ae4104e6bf1b8444324ce89999908fb..9f702f58944ee5699b81369c3289990850a8a819 100644
--- a/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java
+++ b/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java
@@ -60,13 +60,15 @@ final class CronetFixedModeOutputStream extends CronetOutputStream {
     @Override
     public void write(int oneByte) throws IOException {
         checkNotExceedContentLength(1);
-        while (mBuffer.position() == mBuffer.limit()) {

[xunjieli, 2016/06/10 00:10:48]

I can't remember why I put "while" here. I will think more about this tomorrow.

[pauljensen, 2016/06/10 13:27:39]

instead of "position == limit" we should say hasRemaining()

[pauljensen, 2016/06/10 13:27:39]

Actually I like "while" more than "if" because it implies that there will be
space in the buffer after the loop.  I think we should switch to "while" in both
places.

[xunjieli, 2016/06/10 14:05:00]

Done.

+        if (mBuffer.position() == mBuffer.limit()) {
+            mBuffer.flip();
             // Wait until buffer is consumed.
             mMessageLoop.loop();
         }

[pauljensen, 2016/06/10 13:27:39]

We should move this loop into a separate function as it's copied in several
places here, perhaps call it, ensureBufferHasRemaining().

[xunjieli, 2016/06/10 14:05:00]

Done.

         mBuffer.put((byte) oneByte);
         mBytesWritten++;
         if (mBytesWritten == mContentLength) {
+            mBuffer.flip();
             // Entire post data has been received. Now wait for network stack to
             // read it.
             mMessageLoop.loop();
@@ -85,6 +87,7 @@ final class CronetFixedModeOutputStream extends CronetOutputStream {
         int toSend = count;
         while (toSend > 0) {
             if (mBuffer.position() == mBuffer.limit()) {
+                mBuffer.flip();
                 // Wait until buffer is consumed.
                 mMessageLoop.loop();
             }

[pauljensen, 2016/06/10 13:27:39]

This function seems like it could really use a short-circuit to avoid useless
buffer copies.  Something like:
if (toSend >= sDefaultBufferLength && mBuffer.position() == 0) {
   // Send directly from |buffer|, perhaps by swapping it in for |mBuffer|.
}

[xunjieli, 2016/06/10 14:05:00]

Let's keep this as it is now. Swapping it for mBuffer also means that we just
allocated one and decided to throw it away. We can't send directly from |buffer|
unless UploadDataProvider can access it. It just seems that code will be
complicated. Happy to review the CL if you want to give it a try :)

@@ -94,6 +97,7 @@ final class CronetFixedModeOutputStream extends CronetOutputStream {
         }
         mBytesWritten += count;
         if (mBytesWritten == mContentLength) {
+            mBuffer.flip();
             // Entire post data has been received. Now wait for network stack to
             // read it.
             mMessageLoop.loop();
@@ -141,25 +145,20 @@ final class CronetFixedModeOutputStream extends CronetOutputStream {
 
         @Override
         public void read(final UploadDataSink uploadDataSink, final ByteBuffer byteBuffer) {
-            final int availableSpace = byteBuffer.remaining();
-            if (availableSpace < mBuffer.position()) {
-                // byteBuffer does not have enough capacity, so only put a portion
-                // of mBuffer in it.
-                byteBuffer.put(mBuffer.array(), 0, availableSpace);
-                mBuffer.position(availableSpace);
-                // Move remaining buffer to the head of the buffer for use in the
-                // next read call.
-                mBuffer.compact();
-            } else {
-                // byteBuffer has enough capacity to hold the content of mBuffer.
-                mBuffer.flip();
+            if (byteBuffer.remaining() >= mBuffer.remaining()) {
                 byteBuffer.put(mBuffer);
                 // Reuse this buffer.
                 mBuffer.clear();
+                uploadDataSink.onReadSucceeded(false);
                 // Quit message loop so embedder can write more data.
                 mMessageLoop.quit();
+            } else {
+                int oldLimit = mBuffer.limit();
+                mBuffer.limit(mBuffer.position() + byteBuffer.remaining());
+                byteBuffer.put(mBuffer);
+                mBuffer.limit(oldLimit);
+                uploadDataSink.onReadSucceeded(false);
             }
-            uploadDataSink.onReadSucceeded(false);
         }
 
         @Override