[Cronet] Add Hello World response to QuicTestServer and use it in bidirectional stream test.

[Cronet] Add Hello World response to QuicTestServer and use it in bidirectional stream test.

BUG=601972
BUG=606818
Committed: https://crrev.com/5ef58edc3a8a1aa123a75daa8045b6962da2f1c6
Cr-Commit-Position: refs/heads/master@{#390786}

[mef, 2016-04-17 23:08:57]

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

[mef, 2016-04-17 23:08:58]

PTAL, I've added 'Hello world' response to quic test server and used it in
bidirectional stream test for iOS.

[kapishnikov, 2016-04-26 20:11:50]

https://codereview.chromium.org/1892423002/diff/20001/components/cronet/ios/t...
File components/cronet/ios/test/cronet_bidirectional_stream_test.mm (right):

https://codereview.chromium.org/1892423002/diff/20001/components/cronet/ios/t...
components/cronet/ios/test/cronet_bidirectional_stream_test.mm:233:
test->response_trailers[trailers->headers[i].key] =
Is it possible for the response to contain two or more trailers with the same
key? If it is possible, maybe we should consider using a map of lists, i.e. {key
: [value1, value2, etc]}.

[xunjieli, 2016-04-26 20:33:19]

Thanks for adding the trailers test! Is it possible to add an equivalent test of
BidirectionalStreamQuicTest#testStreamFailBeforeWriteIsExecutedOnNetworkThread
(where we shut down the server during a on_write_completed_callback after
calling into write) in this CL or in a follow-up? I believe the code will crash
(crbug.com/606818), and it will be good to write a test to double check.

[mef, 2016-04-26 20:39:22]

I'll try to add a test that Helen has suggested.

https://codereview.chromium.org/1892423002/diff/20001/components/cronet/ios/t...
File components/cronet/ios/test/cronet_bidirectional_stream_test.mm (right):

https://codereview.chromium.org/1892423002/diff/20001/components/cronet/ios/t...
components/cronet/ios/test/cronet_bidirectional_stream_test.mm:233:
test->response_trailers[trailers->headers[i].key] =
On 2016/04/26 20:11:50, kapishnikov wrote:
> Is it possible for the response to contain two or more trailers with the same
> key? If it is possible, maybe we should consider using a map of lists, i.e.
{key
> : [value1, value2, etc]}.

I just confirmed that I've recalled correctly and additional values replace
previous in spdy header block (on the quic test server side).

[mef, 2016-04-27 22:15:38]

Description was changed from

==========
[Cronet] Add Hello World response to QuicTestServer and use it in bidirectional
stream test.

BUG=601972
==========

to

==========
[Cronet] Add Hello World response to QuicTestServer and use it in bidirectional
stream test.

BUG=601972
BUG=606818
==========

[mef, 2016-04-27 22:17:04]

Thanks, PTAL. It turns out that both read and write would fail if remote server
has disappeared. I've added tests and fixed that.

[xunjieli, 2016-04-28 15:23:33]

lgtm

https://codereview.chromium.org/1892423002/diff/80001/components/cronet/ios/c...
File components/cronet/ios/cronet_bidirectional_stream.cc (right):

https://codereview.chromium.org/1892423002/diff/80001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:242:
OnFailed(net::ERR_UNEXPECTED);
If the request has been canceled, we probably could provide ERR_ABORTED as the
better error code.

https://codereview.chromium.org/1892423002/diff/80001/components/cronet/ios/t...
File components/cronet/ios/test/cronet_bidirectional_stream_test.mm (right):

https://codereview.chromium.org/1892423002/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_bidirectional_stream_test.mm:181: static void
on_request_headers_sent_callback(
nit: This callback is changed to on_stream_ready().

https://codereview.chromium.org/1892423002/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_bidirectional_stream_test.mm:184: if
(test->MaybeCancel(stream, ON_REQUEST_HEADERS_SENT))
nit: s/ON_REQUEST_HEADERS_SENT/ON_STREAM_READY

[mef, 2016-04-28 17:11:26]

Thanks!

https://codereview.chromium.org/1892423002/diff/80001/components/cronet/ios/c...
File components/cronet/ios/cronet_bidirectional_stream.cc (right):

https://codereview.chromium.org/1892423002/diff/80001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:242:
OnFailed(net::ERR_UNEXPECTED);
On 2016/04/28 15:23:33, xunjieli wrote:
> If the request has been canceled, we probably could provide ERR_ABORTED as the
> better error code.

That's an interesting point. I think we shouldn't invoke OnFailed at all if
stream is in one one of the terminal states (DONE, CANCELED or FAILED). WDYT?

https://codereview.chromium.org/1892423002/diff/80001/components/cronet/ios/t...
File components/cronet/ios/test/cronet_bidirectional_stream_test.mm (right):

https://codereview.chromium.org/1892423002/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_bidirectional_stream_test.mm:181: static void
on_request_headers_sent_callback(
On 2016/04/28 15:23:33, xunjieli wrote:
> nit: This callback is changed to on_stream_ready().

Done.

https://codereview.chromium.org/1892423002/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_bidirectional_stream_test.mm:184: if
(test->MaybeCancel(stream, ON_REQUEST_HEADERS_SENT))
On 2016/04/28 15:23:33, xunjieli wrote:
> nit: s/ON_REQUEST_HEADERS_SENT/ON_STREAM_READY

Done.

[xunjieli, 2016-04-28 20:20:45]

https://codereview.chromium.org/1892423002/diff/80001/components/cronet/ios/c...
File components/cronet/ios/cronet_bidirectional_stream.cc (right):

https://codereview.chromium.org/1892423002/diff/80001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:184:
delegate_->OnFailed(error);
Does the API guarantee that delegate is only get notified of OnFailed at most
once? If so, maybe we should null out delegate_ here.

https://codereview.chromium.org/1892423002/diff/80001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:242:
OnFailed(net::ERR_UNEXPECTED);
On 2016/04/28 17:11:26, mef wrote:
> On 2016/04/28 15:23:33, xunjieli wrote:
> > If the request has been canceled, we probably could provide ERR_ABORTED as
the
> > better error code.
> 
> That's an interesting point. I think we shouldn't invoke OnFailed at all if
> stream is in one one of the terminal states (DONE, CANCELED or FAILED). WDYT?

I am not sure. Either approach is fine with me as long as we document it in the
API doc and be consistent. We can handle this in a follow-up, since I guess we
will end up changing the API anyway to handle flush.

[mef, 2016-04-28 21:35:38]

The CQ bit was checked by mef@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-28 21:36:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1892423002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1892423002/100001

[commit-bot: I haz the power, 2016-04-28 23:08:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-28 23:08:13]

Dry run: This issue passed the CQ dry run.

[mef, 2016-04-29 21:53:29]

The CQ bit was checked by mef@chromium.org

[mef, 2016-04-29 21:53:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
xunjieli@chromium.org
Link to the patchset: https://codereview.chromium.org/1892423002/#ps120001
(title: "Sync and lint.")

[commit-bot: I haz the power, 2016-04-29 21:53:50]

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

[commit-bot: I haz the power, 2016-04-29 22:35:04]

Description was changed from

==========
[Cronet] Add Hello World response to QuicTestServer and use it in bidirectional
stream test.

BUG=601972
BUG=606818
==========

to

==========
[Cronet] Add Hello World response to QuicTestServer and use it in bidirectional
stream test.

BUG=601972
BUG=606818
==========

[commit-bot: I haz the power, 2016-04-29 22:35:05]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-30 17:29:41]

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

[commit-bot: I haz the power, 2016-04-30 18:03:12]

Description was changed from

==========
[Cronet] Add Hello World response to QuicTestServer and use it in bidirectional
stream test.

BUG=601972
BUG=606818
==========

to

==========
[Cronet] Add Hello World response to QuicTestServer and use it in bidirectional
stream test.

BUG=601972
BUG=606818

Committed: https://crrev.com/5ef58edc3a8a1aa123a75daa8045b6962da2f1c6
Cr-Commit-Position: refs/heads/master@{#390786}
==========