[Cronet] Enable chunked upload in CronetUrlRequest

[Cronet] Enable chunked upload in CronetUrlRequest

Chunked upload support is added for embedded_test_server, so we should
enable chunked upload in CronetUrlRequest since now we have testing support.

BUG=412942
Committed: https://crrev.com/495ad5a3bde13a2938d84862cb841f39220ed4b7
Cr-Commit-Position: refs/heads/master@{#318536}

[xunjieli, 2015-02-25 22:11:10]

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

[xunjieli, 2015-02-25 22:11:11]

https://codereview.chromium.org/948503004/diff/1/components/cronet/android/cr...
File components/cronet/android/cronet_upload_data_stream_adapter.cc (right):

https://codereview.chromium.org/948503004/diff/1/components/cronet/android/cr...
components/cronet/android/cronet_upload_data_stream_adapter.cc:90: // 
DCHECK(!is_chunked() || !final_chunk);
OnReadSuccess is called for final_chunk. This DCHECK doesn't make sense.. Should
I change it to: 

if (!is_chunked()) {
 DCHECK(!final_chunk);
}

?

https://codereview.chromium.org/948503004/diff/1/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/948503004/diff/1/net/url_request/url_request....
net/url_request/url_request.cc:216: //  DCHECK(!upload->is_chunked());
CronetUploadDataStreamAdapter is used for both chunked and non-chunked. So
should I remove this DCHECK?

[mmenke, 2015-02-25 23:58:24]

https://codereview.chromium.org/948503004/diff/1/components/cronet/android/cr...
File components/cronet/android/cronet_upload_data_stream_adapter.cc (right):

https://codereview.chromium.org/948503004/diff/1/components/cronet/android/cr...
components/cronet/android/cronet_upload_data_stream_adapter.cc:90: // 
DCHECK(!is_chunked() || !final_chunk);
On 2015/02/25 22:11:11, xunjieli wrote:
> OnReadSuccess is called for final_chunk. This DCHECK doesn't make sense..
Should
> I change it to: 
> 
> if (!is_chunked()) {
>  DCHECK(!final_chunk);
> }
> 
> ?

Yes, please change it that.  The original code should be is_chunked() rather
than !is_chunked(), but writing it as an if is much clearer.

https://codereview.chromium.org/948503004/diff/1/components/cronet/android/te...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/948503004/diff/1/components/cronet/android/te...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:813:
public void testUploadChunkedNotSupported() throws Exception {
Rename test.  :)

https://codereview.chromium.org/948503004/diff/1/components/cronet/android/te...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:820:
dataProvider.addRead("test hello".getBytes());
Suggest a second test with two reads, and the second having a 0-length body.

https://codereview.chromium.org/948503004/diff/1/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/948503004/diff/1/net/url_request/url_request....
net/url_request/url_request.cc:216: //  DCHECK(!upload->is_chunked());
On 2015/02/25 22:11:11, xunjieli wrote:
> CronetUploadDataStreamAdapter is used for both chunked and non-chunked. So
> should I remove this DCHECK?

Yea, just remove this DCHECK.

[xunjieli, 2015-02-26 20:45:57]

Thanks for the review! I have one question below.

https://codereview.chromium.org/948503004/diff/1/components/cronet/android/cr...
File components/cronet/android/cronet_upload_data_stream_adapter.cc (right):

https://codereview.chromium.org/948503004/diff/1/components/cronet/android/cr...
components/cronet/android/cronet_upload_data_stream_adapter.cc:90: // 
DCHECK(!is_chunked() || !final_chunk);
On 2015/02/25 23:58:24, mmenke wrote:
> On 2015/02/25 22:11:11, xunjieli wrote:
> > OnReadSuccess is called for final_chunk. This DCHECK doesn't make sense..
> Should
> > I change it to: 
> > 
> > if (!is_chunked()) {
> >  DCHECK(!final_chunk);
> > }
> > 
> > ?
> 
> Yes, please change it that.  The original code should be is_chunked() rather
> than !is_chunked(), but writing it as an if is much clearer.

Done.

https://codereview.chromium.org/948503004/diff/1/components/cronet/android/te...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/948503004/diff/1/components/cronet/android/te...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:813:
public void testUploadChunkedNotSupported() throws Exception {
On 2015/02/25 23:58:24, mmenke wrote:
> Rename test.  :)

Oops. thanks!

https://codereview.chromium.org/948503004/diff/20001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/948503004/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:837:
public void testUploadChunkedSecondReadZeroLengthBody() throws Exception {
So the second read isn't considered as final chunk in the TestDataProvider.
Should I change TestDataProvider or should I modify the DCHECK(bytes_read > 0 ||
(final_chunk && bytes_read == 0)) in
https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...
?

[mmenke, 2015-02-26 20:58:38]

https://codereview.chromium.org/948503004/diff/20001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/948503004/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:837:
public void testUploadChunkedSecondReadZeroLengthBody() throws Exception {
On 2015/02/26 20:45:57, xunjieli wrote:
> So the second read isn't considered as final chunk in the TestDataProvider.
> Should I change TestDataProvider or should I modify the DCHECK(bytes_read > 0
||
> (final_chunk && bytes_read == 0)) in
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...
> ? 

That's a bug in the TestDataProvider, so should fix that.

[xunjieli, 2015-02-26 22:15:21]

On 2015/02/26 20:58:38, mmenke wrote:
>
https://codereview.chromium.org/948503004/diff/20001/components/cronet/androi...
> File
>
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
> (right):
> 
>
https://codereview.chromium.org/948503004/diff/20001/components/cronet/androi...
>
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:837:
> public void testUploadChunkedSecondReadZeroLengthBody() throws Exception {
> On 2015/02/26 20:45:57, xunjieli wrote:
> > So the second read isn't considered as final chunk in the TestDataProvider.
> > Should I change TestDataProvider or should I modify the DCHECK(bytes_read >
0
> ||
> > (final_chunk && bytes_read == 0)) in
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...
> > ? 
> 
> That's a bug in the TestDataProvider, so should fix that.

Done. Thanks!

[mmenke, 2015-02-26 22:30:19]

https://codereview.chromium.org/948503004/diff/40001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/net/TestDataProvider.java
(right):

https://codereview.chromium.org/948503004/diff/40001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/net/TestDataProvider.java:124:
|| mNextRead == mReads.size()));
This should actually just be:

final boolean finalChunk = (mChunked && mNextRead == mReads.size() - 1);

And further down, the "else if (!finalChunk)" should just be "else".

The original code always appended an empty final chunk to the end, when it
should have just set finalChunk to true for the last element in mReads.  Think
the original API had a special call to indicate a chunked upload was sent, but
then I changed that (Or I was considering that?).  Anyways, that and your two
tests mean we test both cases.  You'll have to update the number of expected
reads in one of them.

[xunjieli, 2015-02-26 22:53:53]

Thanks for the review!

https://codereview.chromium.org/948503004/diff/40001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/net/TestDataProvider.java
(right):

https://codereview.chromium.org/948503004/diff/40001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/net/TestDataProvider.java:124:
|| mNextRead == mReads.size()));
On 2015/02/26 22:30:19, mmenke wrote:
> This should actually just be:
> 
> final boolean finalChunk = (mChunked && mNextRead == mReads.size() - 1);
> 
> And further down, the "else if (!finalChunk)" should just be "else".
> 
> The original code always appended an empty final chunk to the end, when it
> should have just set finalChunk to true for the last element in mReads.  Think
> the original API had a special call to indicate a chunked upload was sent, but
> then I changed that (Or I was considering that?).  Anyways, that and your two
> tests mean we test both cases.  You'll have to update the number of expected
> reads in one of them.

I see. That totally makes sense. Was a little confused about the extra call.
Done.

[mef, 2015-02-27 16:53:47]

https://codereview.chromium.org/948503004/diff/60001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/948503004/diff/60001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:845:
dataProvider.addRead("hello there".getBytes());
would it make sense to split first read into 2 to test multiple non-empty
chunks?

https://codereview.chromium.org/948503004/diff/60001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:860:

Do we need a test that cancels request between chunks?
Do we need a test that fails read while sending chunks?

[xunjieli, 2015-02-27 19:52:27]

https://codereview.chromium.org/948503004/diff/60001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/948503004/diff/60001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:845:
dataProvider.addRead("hello there".getBytes());
On 2015/02/27 16:53:46, mef wrote:
> would it make sense to split first read into 2 to test multiple non-empty
> chunks?

Done.

https://codereview.chromium.org/948503004/diff/60001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:860:

On 2015/02/27 16:53:46, mef wrote:
> Do we need a test that cancels request between chunks?
> Do we need a test that fails read while sending chunks?

I thought about this, but the chunked mode is pretty much the same as the
non-chunked mode except that it calls uploadDataSink.onReadSucceeded(true) for
the last chunk. So I'm not sure it'd add much if we duplicate test cases like
testUploadReadFailAsync/Sync and testUploadRewindFailSync/Async and add a
setChunked(true).

[mmenke, 2015-02-27 20:44:44]

LGTM!

https://codereview.chromium.org/948503004/diff/80001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/948503004/diff/80001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:821:
dataProvider.addRead(" hello".getBytes());
I actually think we should just have one chunk here (The next one checks
multiple non-empty reads already)

[xunjieli, 2015-02-27 20:57:21]

Thanks!

https://codereview.chromium.org/948503004/diff/80001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/948503004/diff/80001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:821:
dataProvider.addRead(" hello".getBytes());
On 2015/02/27 20:44:44, mmenke wrote:
> I actually think we should just have one chunk here (The next one checks
> multiple non-empty reads already)

Done.

[mef, 2015-02-27 21:23:14]

lgtm

https://codereview.chromium.org/948503004/diff/100001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/948503004/diff/100001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:844:
// Add two reads. The second read has 0-length body.
nit: the comment needs to be updated.

[xunjieli, 2015-02-27 21:32:11]

Thanks for the reviews!

https://codereview.chromium.org/948503004/diff/100001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/948503004/diff/100001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:844:
// Add two reads. The second read has 0-length body.
On 2015/02/27 21:23:13, mef wrote:
> nit: the comment needs to be updated.

Done.

[xunjieli, 2015-02-27 21:32:23]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2015-02-27 21:32:23]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
mef@chromium.org
Link to the patchset: https://codereview.chromium.org/948503004/#ps120001
(title: "Update comments")

[commit-bot: I haz the power, 2015-02-27 21:33:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/948503004/120001

[commit-bot: I haz the power, 2015-02-27 22:56:48]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-27 22:57:25]

Patchset 7 (id:??) landed as
https://crrev.com/495ad5a3bde13a2938d84862cb841f39220ed4b7
Cr-Commit-Position: refs/heads/master@{#318536}