[Cronet] Remove buffer.compact from CronetChunkedOutputStream

[Cronet] Remove buffer.compact from CronetChunkedOutputStream

This CL unifies the buffer manipulation code in CronetChunkedOutputStream to
match the one in CronetFixedOutputStream.

BUG=626653
BUG=618872
Committed: https://crrev.com/bf9178a723b6fb899d7b73f2793d532fc4badd66
Cr-Commit-Position: refs/heads/master@{#404664}

[xunjieli, 2016-07-08 18:26:03]

Description was changed from

==========
[Cronet] Remove buffer.compact from CronetChunkedOutputStream

This CL unifies the buffer manipulation code in CronetChunkedOutputStream to
match the one in CronetFixedOutputStream.

BUG=626653
==========

to

==========
[Cronet] Remove buffer.compact from CronetChunkedOutputStream

This CL unifies the buffer manipulation code in CronetChunkedOutputStream to
match the one in CronetFixedOutputStream.

BUG=626653
BUG=618872
==========

[xunjieli, 2016-07-08 19:56:13]

Patchset #1 (id:1) has been deleted

[xunjieli, 2016-07-08 20:01:07]

Patchset #1 (id:20001) has been deleted

[xunjieli, 2016-07-08 20:30:59]

xunjieli@chromium.org changed reviewers:
+ kapishnikov@chromium.org, mef@chromium.org

[xunjieli, 2016-07-08 20:31:00]

Misha and Andrei: This is based on top of the other CL. This addressed another
bug that I overlooked earlier. This is the chunked equivalent of
crbug.com/618872. PTAL.

https://codereview.chromium.org/2133273002/diff/40001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java
(right):

https://codereview.chromium.org/2133273002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java:108:
if (byteBuffer.remaining() >= mBuffer.remaining()) {
This is done to match the new code in CronetFixedModeOutputStream. I added a
test, which without this patch will keep uploading data to the server. The
reason is that we were previously setting
mBuffer.position(byteBuffer.remaining()) before compacting the buffer. This
causes UploadDataProvider to upload a 16kB chunk over and over again.

[kapishnikov, 2016-07-08 23:05:12]

https://codereview.chromium.org/2133273002/diff/40001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java
(right):

https://codereview.chromium.org/2133273002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java:32:
* @param contentLength The content length of the request body. Non-zero for
contentLength  => chunkLength

https://codereview.chromium.org/2133273002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java:65:
int sent = Math.min(toSend, mBuffer.remaining());
If toSend == mBuffer.remaining(), is it going to wait for the next chunk or
close() before uploading the data? If yes, would it be better to flip the buffer
right away and run the loop?

https://codereview.chromium.org/2133273002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java:75:
if (!mLastChunk) {
Should we make it thread safe?

[xunjieli, 2016-07-11 13:40:08]

Thanks for the review! PTAL.

https://codereview.chromium.org/2133273002/diff/40001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java
(right):

https://codereview.chromium.org/2133273002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java:32:
* @param contentLength The content length of the request body. Non-zero for
On 2016/07/08 23:05:12, kapishnikov wrote:
> contentLength  => chunkLength

Done. Good catch!

https://codereview.chromium.org/2133273002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java:65:
int sent = Math.min(toSend, mBuffer.remaining());
On 2016/07/08 23:05:12, kapishnikov wrote:
> If toSend == mBuffer.remaining(), is it going to wait for the next chunk or
> close() before uploading the data? If yes, would it be better to flip the
buffer
> right away and run the loop?

Done. That is a great suggestion. I agree that it will be better if we run the
loop right away if exactly a chunk is passed to us.

https://codereview.chromium.org/2133273002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java:75:
if (!mLastChunk) {
On 2016/07/08 23:05:12, kapishnikov wrote:
> Should we make it thread safe?

HttpURLConnection is not thread-safe. The APIs are supposed to be used from the
same thread. So I guess the answer is no?

[kapishnikov, 2016-07-11 15:12:42]

LGTM

[xunjieli, 2016-07-11 15:17:36]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2016-07-11 15:17:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 15:53:41]

Description was changed from

==========
[Cronet] Remove buffer.compact from CronetChunkedOutputStream

This CL unifies the buffer manipulation code in CronetChunkedOutputStream to
match the one in CronetFixedOutputStream.

BUG=626653
BUG=618872
==========

to

==========
[Cronet] Remove buffer.compact from CronetChunkedOutputStream

This CL unifies the buffer manipulation code in CronetChunkedOutputStream to
match the one in CronetFixedOutputStream.

BUG=626653
BUG=618872
==========

[commit-bot: I haz the power, 2016-07-11 15:53:43]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-07-11 15:56:03]

Description was changed from

==========
[Cronet] Remove buffer.compact from CronetChunkedOutputStream

This CL unifies the buffer manipulation code in CronetChunkedOutputStream to
match the one in CronetFixedOutputStream.

BUG=626653
BUG=618872
==========

to

==========
[Cronet] Remove buffer.compact from CronetChunkedOutputStream

This CL unifies the buffer manipulation code in CronetChunkedOutputStream to
match the one in CronetFixedOutputStream.

BUG=626653
BUG=618872

Committed: https://crrev.com/bf9178a723b6fb899d7b73f2793d532fc4badd66
Cr-Commit-Position: refs/heads/master@{#404664}
==========

[commit-bot: I haz the power, 2016-07-11 15:56:03]

Patchset 2 (id:??) landed as
https://crrev.com/bf9178a723b6fb899d7b73f2793d532fc4badd66
Cr-Commit-Position: refs/heads/master@{#404664}