Implement QUIC-based net::BidirectionalStream

Implement QUIC-based net::BidirectionalStream

This CL implements a QUIC-based BidirectionalStream
and adds unit tests.

BUG=584338
Committed: https://crrev.com/0527bf95252b833b591be2f8e116cffd004ff189
Cr-Commit-Position: refs/heads/master@{#380919}

Committed: https://crrev.com/2608f9bb792efa1a90730000038d518cd5bca400
Cr-Commit-Position: refs/heads/master@{#380965}

[xunjieli, 2016-02-26 23:16:33]

Description was changed from

==========
temp2

BUG=
==========

to

==========
Implement QUIC-based net::BidirectionalStreamJob

This CL implements a QUIC-based BidirectionalStreamJob
and adds unit tests.

BUG=584338
==========

[xunjieli, 2016-02-26 23:16:41]

Patchset #1 (id:1) has been deleted

[xunjieli, 2016-02-26 23:30:32]

Patchset #1 (id:20001) has been deleted

[xunjieli, 2016-02-26 23:34:36]

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

[xunjieli, 2016-02-26 23:34:36]

Ryan and Misha: PTAL. (This depends on the trailers CL. But they should be
largely independent.) Thanks!

[Ryan Hamilton, 2016-02-27 00:21:15]

I'd like to back up and talk about the #ifdef approach to this bidirectional
code.

https://codereview.chromium.org/1744693002/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/1744693002/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:58: proxy_ssl_config, delegate, nullptr,
false,
"false" is not super readable here ("what is false?") How about:

/*for_bidirecitonal=*/false

(and elsewhere)

https://codereview.chromium.org/1744693002/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:106: : Request::HTTP_STREAM);
oh, since you already have an enum, just do that instead of the bool. That'll
make the literals easy to read.

https://codereview.chromium.org/1744693002/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.h (right):

https://codereview.chromium.org/1744693002/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.h:87: bool for_bidirectional,
how about bidirectional_stream or some such?

https://codereview.chromium.org/1744693002/diff/40001/net/quic/bidirectional_...
File net/quic/bidirectional_stream_quic_job.cc (right):

https://codereview.chromium.org/1744693002/diff/40001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_job.cc:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
It looks like this class has a lot of duplicate code with QuicHttpSteam. Can it
be refactored so they share code?

https://codereview.chromium.org/1744693002/diff/40001/net/quic/bidirectional_...
File net/quic/bidirectional_stream_quic_job.h (right):

https://codereview.chromium.org/1744693002/diff/40001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_job.h:21: #endif
I am really not a fan of this conditional compilation stuff. It makes code
search not work and it means that local builds after refactors do not work.
Please consider undoing this decision!

https://codereview.chromium.org/1744693002/diff/40001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_job.h:34: : public BidirectionalStreamJob,
It doesn't seem like BidirectionalStreamJob really represents a "Job". I think
of a job as something like HttpStreamFactoryImpl::Job which is start in order to
do something, create a stream, and when the job is finished, the thing is
available to be used. In this case, if I'm reading the code correctly, the "Job"
is required for the life of the BidirectionalStream. Is that right? Perhaps
instead of Job it should be Helper, or Impl, or something.

https://codereview.chromium.org/1744693002/diff/40001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1744693002/diff/40001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:567: #endif
I find this maze of ifdefs exceptionally hard to read. I think we need to back
up and see if we can find a different solution.

https://codereview.chromium.org/1744693002/diff/40001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1744693002/diff/40001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.h:95: bool for_bidirectional);
If we need a new argument, just add it to the existing method.

https://codereview.chromium.org/1744693002/diff/40001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.h:107: #endif
oh man, I really really really don't like this #ifdef all over the place.

[xunjieli, 2016-02-29 15:21:38]

Thanks for the review! I agree that the ifdefs is pretty ugly. But we are using
net feature flags (see build/buildflag_header.gni) which is an improvement from
the plain old ifdefs. It was initially added to not bloat Chrome binary since
the API is still in experimental stage. I am open to suggestions.

https://codereview.chromium.org/1744693002/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/1744693002/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:58: proxy_ssl_config, delegate, nullptr,
false,
On 2016/02/27 00:21:14, Ryan Hamilton wrote:
> "false" is not super readable here ("what is false?") How about:
> 
> /*for_bidirecitonal=*/false
> 
> (and elsewhere)

Done.

https://codereview.chromium.org/1744693002/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:106: : Request::HTTP_STREAM);
On 2016/02/27 00:21:14, Ryan Hamilton wrote:
> oh, since you already have an enum, just do that instead of the bool. That'll
> make the literals easy to read.

Done. Great idea!

https://codereview.chromium.org/1744693002/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.h (right):

https://codereview.chromium.org/1744693002/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.h:87: bool for_bidirectional,
On 2016/02/27 00:21:14, Ryan Hamilton wrote:
> how about bidirectional_stream or some such?

Done.

https://codereview.chromium.org/1744693002/diff/40001/net/quic/bidirectional_...
File net/quic/bidirectional_stream_quic_job.cc (right):

https://codereview.chromium.org/1744693002/diff/40001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_job.cc:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
On 2016/02/27 00:21:14, Ryan Hamilton wrote:
> It looks like this class has a lot of duplicate code with QuicHttpSteam. Can
it
> be refactored so they share code?

I'd like to think this is a thin wrapper on top of QuicChromiumClientStream.
QuicHttpStream is slightly more complicated. Since this is pretty much in
experimental stage, I don't think I should touch QuicHttpStream (similarly
SpdyHttpStream)? Maybe when the API matures, we can refactor them?

https://codereview.chromium.org/1744693002/diff/40001/net/quic/bidirectional_...
File net/quic/bidirectional_stream_quic_job.h (right):

https://codereview.chromium.org/1744693002/diff/40001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_job.h:21: #endif
On 2016/02/27 00:21:14, Ryan Hamilton wrote:
> I am really not a fan of this conditional compilation stuff. It makes code
> search not work and it means that local builds after refactors do not work.
> Please consider undoing this decision! 

We used build flag to exclude bidirectional stream related code so as not to
bloat Chrome binary. I agree it is pretty ugly, but I am not sure if there's
another option.

https://codereview.chromium.org/1744693002/diff/40001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_job.h:34: : public BidirectionalStreamJob,
On 2016/02/27 00:21:14, Ryan Hamilton wrote:
> It doesn't seem like BidirectionalStreamJob really represents a "Job". I think
> of a job as something like HttpStreamFactoryImpl::Job which is start in order
to
> do something, create a stream, and when the job is finished, the thing is
> available to be used. In this case, if I'm reading the code correctly, the
"Job"
> is required for the life of the BidirectionalStream. Is that right? Perhaps
> instead of Job it should be Helper, or Impl, or something.

Done. I agree. I have changed the naming for new files in this CL. I will do a
follow-up CL to rename the old files so as to keep the size of the CL small.

https://codereview.chromium.org/1744693002/diff/40001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1744693002/diff/40001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:567: #endif
On 2016/02/27 00:21:14, Ryan Hamilton wrote:
> I find this maze of ifdefs exceptionally hard to read. I think we need to back
> up and see if we can find a different solution.

Acknowledged.

https://codereview.chromium.org/1744693002/diff/40001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1744693002/diff/40001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.h:95: bool for_bidirectional);
On 2016/02/27 00:21:14, Ryan Hamilton wrote:
> If we need a new argument, just add it to the existing method.

Done.

https://codereview.chromium.org/1744693002/diff/40001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.h:107: #endif
On 2016/02/27 00:21:14, Ryan Hamilton wrote:
> oh man, I really really really don't like this #ifdef all over the place.

Acknowledged. I agree it is pretty ugly. Do you have any suggestion?

[xunjieli, 2016-02-29 15:50:54]

Description was changed from

==========
Implement QUIC-based net::BidirectionalStreamJob

This CL implements a QUIC-based BidirectionalStreamJob
and adds unit tests.

BUG=584338
==========

to

==========
Implement QUIC-based net::BidirectionalStream

This CL implements a QUIC-based BidirectionalStream
and adds unit tests.

BUG=584338
==========

[xunjieli, 2016-02-29 15:51:45]

+ rjshade@

[mef, 2016-02-29 16:26:52]

https://codereview.chromium.org/1744693002/diff/60001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/1744693002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_unittest.cc:753: // a
BidirectionalStreamJob::Delegate to wait until response headers are
nit: loose 'a'?

https://codereview.chromium.org/1744693002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_unittest.cc:1526: maker_(GetParam(), 0,
clock_, "www.example.org"),
nit: maker seems a bit too generic, maybe call it packet_maker or
test_packet_maker? Both variable and accessor.

https://codereview.chromium.org/1744693002/diff/60001/net/quic/bidirectional_...
File net/quic/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/1744693002/diff/60001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_impl.cc:134: total_received_bytes +=
stream_->stream_bytes_read();
Could just "return headers_bytes_received_ + stream_->stream_bytes_read();"
here?
and do "return headers_bytes_received_+ closed_stream_received_bytes_" below?

Similarly for sent bytes?

[xunjieli, 2016-02-29 16:34:09]

Thanks for the review! PTAL.

https://codereview.chromium.org/1744693002/diff/60001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/1744693002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_unittest.cc:753: // a
BidirectionalStreamJob::Delegate to wait until response headers are
On 2016/02/29 16:26:52, mef wrote:
> nit: loose 'a'?

Done.

https://codereview.chromium.org/1744693002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_unittest.cc:1526: maker_(GetParam(), 0,
clock_, "www.example.org"),
On 2016/02/29 16:26:52, mef wrote:
> nit: maker seems a bit too generic, maybe call it packet_maker or
> test_packet_maker? Both variable and accessor.

Done.

https://codereview.chromium.org/1744693002/diff/60001/net/quic/bidirectional_...
File net/quic/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/1744693002/diff/60001/net/quic/bidirectional_...
net/quic/bidirectional_stream_quic_impl.cc:134: total_received_bytes +=
stream_->stream_bytes_read();
On 2016/02/29 16:26:52, mef wrote:
> Could just "return headers_bytes_received_ + stream_->stream_bytes_read();"
> here?
> and do "return headers_bytes_received_+ closed_stream_received_bytes_" below?
> 
> Similarly for sent bytes?

Done.

[xunjieli, 2016-03-02 16:43:39]

Thanks Ryan for reviewing the other trailers CL! Since that is landed, friendly
pinging for review on this one.

[Ryan Hamilton, 2016-03-03 04:14:58]

https://codereview.chromium.org/1744693002/diff/120001/net/http/http_stream_f...
File net/http/http_stream_factory.h (right):

https://codereview.chromium.org/1744693002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory.h:59: BIDIRECTIONAL_STREAM_SPDY_JOB,
This name is pretty long. How about just BIDIRECTIONAL_STREAM?

https://codereview.chromium.org/1744693002/diff/120001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1744693002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:1021: for_bidirectional_);
Let's use HttpStreamRequest::StreamType stream_type instead off bool
for_bidirectional.

https://codereview.chromium.org/1744693002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:1264: #endif
I don't want to keep harping on this, but this is SO ugly. Here we have a mix of
an always-compiled boolean, which triggers an ifdef-only codepath. This just
feels so gross. Who can we talk to about revisiting the decision to ifdef this?

https://codereview.chromium.org/1744693002/diff/120001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1744693002/diff/120001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:542: DCHECK(stream_ || (for_bidirectional_ &&
bidirectional_stream_job_));
This would be true if both stream_ and bidirectional_stream_job_ were both set.
Is that desirable?

https://codereview.chromium.org/1744693002/diff/120001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:562: }
How about instead of creating the QuicHttpStream or Bidirectional stream here,
and then storing it in one of two member, how about change ReleaseStream() to
create this object on the fly?

https://codereview.chromium.org/1744693002/diff/120001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1744693002/diff/120001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:85: bool for_bidirectional);
Ditto about using the enum instead of the boolean. Though if you take my advice,
you might be able to get rid of this variable completely by moving some logic to
the Release method.

https://codereview.chromium.org/1744693002/diff/120001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:118: bool for_bidirectional_;
Should this be inside this #ifdef?

[xunjieli, 2016-03-03 16:53:55]

Patchset #6 (id:140001) has been deleted

[xunjieli, 2016-03-03 16:54:20]

Looks much better than the last patch. Thanks! PTAL.

https://codereview.chromium.org/1744693002/diff/120001/net/http/http_stream_f...
File net/http/http_stream_factory.h (right):

https://codereview.chromium.org/1744693002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory.h:59: BIDIRECTIONAL_STREAM_SPDY_JOB,
On 2016/03/03 04:14:57, Ryan Hamilton wrote:
> This name is pretty long. How about just BIDIRECTIONAL_STREAM?

Done.

https://codereview.chromium.org/1744693002/diff/120001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1744693002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:1021: for_bidirectional_);
On 2016/03/03 04:14:57, Ryan Hamilton wrote:
> Let's use HttpStreamRequest::StreamType stream_type instead off bool
> for_bidirectional.

Done.

https://codereview.chromium.org/1744693002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:1264: #endif
On 2016/03/03 04:14:57, Ryan Hamilton wrote:
> I don't want to keep harping on this, but this is SO ugly. Here we have a mix
of
> an always-compiled boolean, which triggers an ifdef-only codepath. This just
> feels so gross. Who can we talk to about revisiting the decision to ifdef
this?

After changing for_bidirectional_ to the enum, is this slightly better?

https://codereview.chromium.org/1744693002/diff/120001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1744693002/diff/120001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:542: DCHECK(stream_ || (for_bidirectional_ &&
bidirectional_stream_job_));
On 2016/03/03 04:14:57, Ryan Hamilton wrote:
> This would be true if both stream_ and bidirectional_stream_job_ were both
set.
> Is that desirable?

Done.

https://codereview.chromium.org/1744693002/diff/120001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:562: }
On 2016/03/03 04:14:58, Ryan Hamilton wrote:
> How about instead of creating the QuicHttpStream or Bidirectional stream here,
> and then storing it in one of two member, how about change ReleaseStream() to
> create this object on the fly?

Done. Great idea! But this would change the semantics of ReleaseStream(), which
now sometimes can return nullptr if session is closed before ReleaseStream() is
invoked. Will that be okay? See quic_stream_factory_test.cc for an example.

https://codereview.chromium.org/1744693002/diff/120001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1744693002/diff/120001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:85: bool for_bidirectional);
On 2016/03/03 04:14:58, Ryan Hamilton wrote:
> Ditto about using the enum instead of the boolean. Though if you take my
advice,
> you might be able to get rid of this variable completely by moving some logic
to
> the Release method.

Done.

https://codereview.chromium.org/1744693002/diff/120001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:118: bool for_bidirectional_;
On 2016/03/03 04:14:58, Ryan Hamilton wrote:
> Should this be inside this #ifdef?

Done.

[xunjieli, 2016-03-07 16:03:44]

A friendly ping..

[Ryan Hamilton, 2016-03-07 20:12:37]

Looking much better.

https://codereview.chromium.org/1744693002/diff/120001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1744693002/diff/120001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:562: }
On 2016/03/03 16:54:19, xunjieli wrote:
> On 2016/03/03 04:14:58, Ryan Hamilton wrote:
> > How about instead of creating the QuicHttpStream or Bidirectional stream
here,
> > and then storing it in one of two member, how about change ReleaseStream()
to
> > create this object on the fly?
> 
> Done. Great idea! But this would change the semantics of ReleaseStream(),
which
> now sometimes can return nullptr if session is closed before ReleaseStream()
is
> invoked. Will that be okay? See quic_stream_factory_test.cc for an example.

Good point. I think you'll need to change
HttpStreamFactoryImpl::Job::DoInitConnectionComplete to detect this case. Looks
like it should be pretty simple, though.

https://codereview.chromium.org/1744693002/diff/160001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1744693002/diff/160001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:1328: DCHECK(false);
nit: this should probably be NOTREACHED();

https://codereview.chromium.org/1744693002/diff/160001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1744693002/diff/160001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:93: scoped_ptr<QuicHttpStream> ReleaseStream();
nit: Now that these don't simply release an existing stream, but create a new
one one the fly, this should probably be renamed CreateStream. (Same below).

[xunjieli, 2016-03-08 15:39:32]

Thanks Ryan! PTAL.

https://codereview.chromium.org/1744693002/diff/120001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1744693002/diff/120001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:562: }
On 2016/03/07 20:12:37, Ryan Hamilton wrote:
> On 2016/03/03 16:54:19, xunjieli wrote:
> > On 2016/03/03 04:14:58, Ryan Hamilton wrote:
> > > How about instead of creating the QuicHttpStream or Bidirectional stream
> here,
> > > and then storing it in one of two member, how about change ReleaseStream()
> to
> > > create this object on the fly?
> > 
> > Done. Great idea! But this would change the semantics of ReleaseStream(),
> which
> > now sometimes can return nullptr if session is closed before ReleaseStream()
> is
> > invoked. Will that be okay? See quic_stream_factory_test.cc for an example.
> 
> Good point. I think you'll need to change
> HttpStreamFactoryImpl::Job::DoInitConnectionComplete to detect this case.
Looks
> like it should be pretty simple, though.

Done.

https://codereview.chromium.org/1744693002/diff/160001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1744693002/diff/160001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:1328: DCHECK(false);
On 2016/03/07 20:12:37, Ryan Hamilton wrote:
> nit: this should probably be NOTREACHED();

Done.

https://codereview.chromium.org/1744693002/diff/160001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1744693002/diff/160001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:93: scoped_ptr<QuicHttpStream> ReleaseStream();
On 2016/03/07 20:12:37, Ryan Hamilton wrote:
> nit: Now that these don't simply release an existing stream, but create a new
> one one the fly, this should probably be renamed CreateStream. (Same below).

Done.

[Ryan Hamilton, 2016-03-08 23:06:09]

lgtm

[mef, 2016-03-08 23:08:21]

On 2016/03/08 23:06:09, Ryan Hamilton wrote:
> lgtm

Yay! I'll look tomorrow, but last time I've looked I could not come up with any
good comments.

[xunjieli, 2016-03-08 23:12:23]

On 2016/03/08 23:08:21, mef wrote:
> On 2016/03/08 23:06:09, Ryan Hamilton wrote:
> > lgtm
> 
> Yay! I'll look tomorrow, but last time I've looked I could not come up with
any
> good comments.

Thanks! I will wait until Misha gets a chance to look. 
Btw, Ryan this CL depends on https://codereview.chromium.org/1774703003. Robbie
is out sick, so he might not get to that.

[mef, 2016-03-09 17:20:37]

Looks pretty good, just few nits and questions.

Also, maybe as a separate CL, but IIRC we need a flag to control whether or not
to request quic bidirectional stream even if quic is available (for experiments
and troubleshooting).

https://codereview.chromium.org/1744693002/diff/180001/net/quic/bidirectional...
File net/quic/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/1744693002/diff/180001/net/quic/bidirectional...
net/quic/bidirectional_stream_quic_impl.cc:72: int
BidirectionalStreamQuicImpl::ReadData(IOBuffer* buf, int buf_len) {
nit: maybe rename buf -> buffer, buf_len -> buffer_len?

https://codereview.chromium.org/1744693002/diff/180001/net/quic/bidirectional...
net/quic/bidirectional_stream_quic_impl.cc:83: stream_->OnFinRead();  // If
write side is close, will call OnClose.
nit: could you rephrase "If write side is close"? 
Also maybe move it up to separate line or get rid of curly braces.

https://codereview.chromium.org/1744693002/diff/180001/net/quic/bidirectional...
net/quic/bidirectional_stream_quic_impl.cc:97: DCHECK(stream_);
what if stream is closed?
I'd say that we need some if(!stream_) return here.

https://codereview.chromium.org/1744693002/diff/180001/net/quic/bidirectional...
net/quic/bidirectional_stream_quic_impl.cc:99: if (length > 0 || end_stream) {
what happens if length < 0 and end_stream is set?
Maybe make it more explicit: if (length > 0 || (length == 0 && end_stream))

https://codereview.chromium.org/1744693002/diff/180001/net/quic/bidirectional...
net/quic/bidirectional_stream_quic_impl.cc:105: DCHECK(rv == OK || rv ==
ERR_IO_PENDING);
could WriteStreamData return an error?

https://codereview.chromium.org/1744693002/diff/180001/net/quic/bidirectional...
File net/quic/bidirectional_stream_quic_impl.h (right):

https://codereview.chromium.org/1744693002/diff/180001/net/quic/bidirectional...
net/quic/bidirectional_stream_quic_impl.h:14: #include
"net/http/bidirectional_stream_request_info.h"
nit: probably can just forward-declare BidirectionalStreamRequestInfo.

[xunjieli, 2016-03-09 21:14:51]

Patchset #8 (id:200001) has been deleted

[xunjieli, 2016-03-09 21:34:54]

Thanks, PTAL.

https://codereview.chromium.org/1744693002/diff/180001/net/quic/bidirectional...
File net/quic/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/1744693002/diff/180001/net/quic/bidirectional...
net/quic/bidirectional_stream_quic_impl.cc:72: int
BidirectionalStreamQuicImpl::ReadData(IOBuffer* buf, int buf_len) {
On 2016/03/09 17:20:37, mef wrote:
> nit: maybe rename buf -> buffer, buf_len -> buffer_len?

Done.

https://codereview.chromium.org/1744693002/diff/180001/net/quic/bidirectional...
net/quic/bidirectional_stream_quic_impl.cc:83: stream_->OnFinRead();  // If
write side is close, will call OnClose.
On 2016/03/09 17:20:37, mef wrote:
> nit: could you rephrase "If write side is close"? 
> Also maybe move it up to separate line or get rid of curly braces.

Done. Rephrased but didn't get rid of the braces, since the comment is for
OnFinRead not for the conditional.

https://codereview.chromium.org/1744693002/diff/180001/net/quic/bidirectional...
net/quic/bidirectional_stream_quic_impl.cc:97: DCHECK(stream_);
On 2016/03/09 17:20:37, mef wrote:
> what if stream is closed?
> I'd say that we need some if(!stream_) return here.

BidirectionalStreamSpdyJob also does a DCHECK. We shouldn't return early here,
since caller might wait for the callback forever. Caller shouldn't call into the
stream when it is destroyed, which probably indicates a programming error.

https://codereview.chromium.org/1744693002/diff/180001/net/quic/bidirectional...
net/quic/bidirectional_stream_quic_impl.cc:99: if (length > 0 || end_stream) {
On 2016/03/09 17:20:37, mef wrote:
> what happens if length < 0 and end_stream is set?
> Maybe make it more explicit: if (length > 0 || (length == 0 && end_stream))

Good catch. I think this should be a DCHECK and not a conditional.

https://codereview.chromium.org/1744693002/diff/180001/net/quic/bidirectional...
net/quic/bidirectional_stream_quic_impl.cc:105: DCHECK(rv == OK || rv ==
ERR_IO_PENDING);
On 2016/03/09 17:20:37, mef wrote:
> could WriteStreamData return an error?

Currently it only returns OK or ERR_IO_PENDING.
https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_chro...

https://codereview.chromium.org/1744693002/diff/180001/net/quic/bidirectional...
File net/quic/bidirectional_stream_quic_impl.h (right):

https://codereview.chromium.org/1744693002/diff/180001/net/quic/bidirectional...
net/quic/bidirectional_stream_quic_impl.h:14: #include
"net/http/bidirectional_stream_request_info.h"
On 2016/03/09 17:20:37, mef wrote:
> nit: probably can just forward-declare BidirectionalStreamRequestInfo.

Done.

[mef, 2016-03-10 19:56:26]

lgtm, thanks!

[xunjieli, 2016-03-10 20:17:40]

On 2016/03/10 19:56:26, mef wrote:
> lgtm, thanks!

Thanks! I will land this once I land the dependent CL
https://codereview.chromium.org/1776423004/. After that, I will do a separate CL
to add the toggle.

[xunjieli, 2016-03-11 21:13:09]

Patchset #10 (id:260001) has been deleted

[xunjieli, 2016-03-14 00:49:22]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-03-14 00:49:22]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org,
mef@chromium.org
Link to the patchset: https://codereview.chromium.org/1744693002/#ps300001
(title: "Rebased")

[commit-bot: I haz the power, 2016-03-14 00:49:30]

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

[commit-bot: I haz the power, 2016-03-14 00:55:56]

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

[commit-bot: I haz the power, 2016-03-14 00:55:57]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[xunjieli, 2016-03-14 01:09:06]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-03-14 01:09:06]

The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org,
rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1744693002/#ps320001
(title: "Rebased again")

[commit-bot: I haz the power, 2016-03-14 01:09:18]

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

[commit-bot: I haz the power, 2016-03-14 02:25:55]

Description was changed from

==========
Implement QUIC-based net::BidirectionalStream

This CL implements a QUIC-based BidirectionalStream
and adds unit tests.

BUG=584338
==========

to

==========
Implement QUIC-based net::BidirectionalStream

This CL implements a QUIC-based BidirectionalStream
and adds unit tests.

BUG=584338
==========

[commit-bot: I haz the power, 2016-03-14 02:25:56]

Committed patchset #12 (id:320001)

[commit-bot: I haz the power, 2016-03-14 02:28:17]

Description was changed from

==========
Implement QUIC-based net::BidirectionalStream

This CL implements a QUIC-based BidirectionalStream
and adds unit tests.

BUG=584338
==========

to

==========
Implement QUIC-based net::BidirectionalStream

This CL implements a QUIC-based BidirectionalStream
and adds unit tests.

BUG=584338

Committed: https://crrev.com/0527bf95252b833b591be2f8e116cffd004ff189
Cr-Commit-Position: refs/heads/master@{#380919}
==========

[commit-bot: I haz the power, 2016-03-14 02:28:19]

Patchset 12 (id:??) landed as
https://crrev.com/0527bf95252b833b591be2f8e116cffd004ff189
Cr-Commit-Position: refs/heads/master@{#380919}

[xunjieli, 2016-03-14 03:14:36]

A revert of this CL (patchset #12 id:320001) has been created in
https://codereview.chromium.org/1793273004/ by xunjieli@chromium.org.

The reason for reverting is: This caused compile error on Cronet bots. .

[xunjieli, 2016-03-14 12:25:18]

Description was changed from

==========
Implement QUIC-based net::BidirectionalStream

This CL implements a QUIC-based BidirectionalStream
and adds unit tests.

BUG=584338

Committed: https://crrev.com/0527bf95252b833b591be2f8e116cffd004ff189
Cr-Commit-Position: refs/heads/master@{#380919}
==========

to

==========
Implement QUIC-based net::BidirectionalStream

This CL implements a QUIC-based BidirectionalStream
and adds unit tests.

BUG=584338

Committed: https://crrev.com/0527bf95252b833b591be2f8e116cffd004ff189
Cr-Commit-Position: refs/heads/master@{#380919}
==========

[xunjieli, 2016-03-14 12:25:25]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-03-14 12:25:25]

The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org,
rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1744693002/#ps340001
(title: "Fix compile due to merge")

[commit-bot: I haz the power, 2016-03-14 12:25:39]

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

[commit-bot: I haz the power, 2016-03-14 13:39:34]

Description was changed from

==========
Implement QUIC-based net::BidirectionalStream

This CL implements a QUIC-based BidirectionalStream
and adds unit tests.

BUG=584338

Committed: https://crrev.com/0527bf95252b833b591be2f8e116cffd004ff189
Cr-Commit-Position: refs/heads/master@{#380919}
==========

to

==========
Implement QUIC-based net::BidirectionalStream

This CL implements a QUIC-based BidirectionalStream
and adds unit tests.

BUG=584338

Committed: https://crrev.com/0527bf95252b833b591be2f8e116cffd004ff189
Cr-Commit-Position: refs/heads/master@{#380919}
==========

[commit-bot: I haz the power, 2016-03-14 13:39:36]

Committed patchset #13 (id:340001)

[commit-bot: I haz the power, 2016-03-14 13:41:18]

Description was changed from

==========
Implement QUIC-based net::BidirectionalStream

This CL implements a QUIC-based BidirectionalStream
and adds unit tests.

BUG=584338

Committed: https://crrev.com/0527bf95252b833b591be2f8e116cffd004ff189
Cr-Commit-Position: refs/heads/master@{#380919}
==========

to

==========
Implement QUIC-based net::BidirectionalStream

This CL implements a QUIC-based BidirectionalStream
and adds unit tests.

BUG=584338

Committed: https://crrev.com/0527bf95252b833b591be2f8e116cffd004ff189
Cr-Commit-Position: refs/heads/master@{#380919}

Committed: https://crrev.com/2608f9bb792efa1a90730000038d518cd5bca400
Cr-Commit-Position: refs/heads/master@{#380965}
==========

[commit-bot: I haz the power, 2016-03-14 13:41:19]

Patchset 13 (id:??) landed as
https://crrev.com/2608f9bb792efa1a90730000038d518cd5bca400
Cr-Commit-Position: refs/heads/master@{#380965}