Allow URLRequest::AppendChunkToUpload to take 0-byte final chunks.

Allow URLRequest::AppendChunkToUpload to take 0-byte final chunks.

Also add HttpStreamParser unit tests for 0 byte final chunks.

BUG=405678
Committed: https://crrev.com/3dc8c88b77aadf129ce466f67d05d0f950c904a2
Cr-Commit-Position: refs/heads/master@{#317051}

[mmenke, 2015-02-11 19:42:04]

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

[mmenke, 2015-02-11 19:42:04]

Finally got back to this.  So little work after...erm...6 months on the back
burner.

I'd like to remove URLRequest::EnableChunkedUpload at some point, keeping the
corresponding method in URLFetcher.  It'll just track the
UploadChunkedDataStream pointer itself.

[mmenke, 2015-02-11 19:43:56]

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

https://codereview.chromium.org/921453003/diff/1/net/url_request/url_request....
net/url_request/url_request.cc:212: DCHECK(upload_data_stream_->is_chunked());
And one quick note...ChunkedUploadDataStream already has a bytes len can only be
0 for the final chunk DCHECK, so no reason to have it here.

[mmenke, 2015-02-11 20:02:30]

Patchset #2 (id:20001) has been deleted

[mef, 2015-02-11 21:32:10]

https://codereview.chromium.org/921453003/diff/1/net/http/http_stream_parser_...
File net/http/http_stream_parser_unittest.cc (right):

https://codereview.chromium.org/921453003/diff/1/net/http/http_stream_parser_...
net/http/http_stream_parser_unittest.cc:376: request_info.upload_data_stream =
&upload_stream;
is it possible to set upload_data_stream to stream which doesn't have any data
yet?

[mmenke, 2015-02-11 22:37:22]

https://codereview.chromium.org/921453003/diff/1/net/http/http_stream_parser_...
File net/http/http_stream_parser_unittest.cc (right):

https://codereview.chromium.org/921453003/diff/1/net/http/http_stream_parser_...
net/http/http_stream_parser_unittest.cc:376: request_info.upload_data_stream =
&upload_stream;

On 2015/02/11 21:32:10, mef wrote:
> is it possible to set upload_data_stream to stream which doesn't have any data
> yet?

Yes...SyncEmptyChunkedUpload is the only test in the file that does that.  We
really don't have many tests for the HttpStreamParser, sadly.  I've added a
couple basic sync/async tests.

Also noticed a silly bug in the logic to connect the mock socket in these tests,
which I've fixed.

[mmenke, 2015-02-11 23:55:03]

I don't feel like I gave you a very good idea of context when you asked me. 
Basically, GSA wanted to be able to send 0-length final chunks, which the API
didn't support.  I added the ability to do that and tests for SPDY and QUIC.  We
didn't have tests for HTTP, but it worked.

I was going to reword UploadDataStream to support it as well, but needed to do
more significant refactors to support streaming data from Java without caching
it, as some embedders wanted, so when I split apart the chunked and non-chunked
stream subclasses while doing that, I made ChunkedUploadDataStream so it would
allow 0-length chunks as well, to fit their use case.

However, I never updated UrlRequest so it would support 0-length final chunks
(Which is needed for the old interface to allow it in debug builds, as well as
to support it for other embedders using the URLRequest::AppendChunkToUpload
method).  This CL finally does that, and adds tests to the HTTP case as well (I
added them to SPDY and QUIC already, as mentioned above).

[mef, 2015-02-12 22:07:40]

Thanks for the explanation! I still need to carefully understand test framework
and scenarios.

[mmenke, 2015-02-12 22:11:13]

On 2015/02/12 22:07:40, mef wrote:
> Thanks for the explanation! I still need to carefully understand test
framework
> and scenarios.

Sure.  There's no rush on this one, so take your time.

[mef, 2015-02-19 14:30:38]

lgtm

[mmenke, 2015-02-19 15:08:44]

On 2015/02/19 14:30:38, mef wrote:
> lgtm

Thanks!  Was going to ping you after simplifying the tests, but wanted to let
the bots cycle first.

[mmenke, 2015-02-19 15:08:49]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2015-02-19 15:09:01]

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

[commit-bot: I haz the power, 2015-02-19 15:11:38]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-02-19 15:12:10]

Patchset 4 (id:??) landed as
https://crrev.com/3dc8c88b77aadf129ce466f67d05d0f950c904a2
Cr-Commit-Position: refs/heads/master@{#317051}