Create a QuicChromiumClientStream::Handle class for allowing a stream

Create a QuicChromiumClientStream::Handle class for allowing a stream
to a be accessed even after it's deleted. This will allow the upcalls
to be removed in subsequent CLs.

This does NOT introduce any new semantics between the Bidi/Http stream and the QUIC stream. It simply adds a layer of indirection between them. Subsequent CLs will change semantics, switchings from upcalls to async IO. But this CL is big enough as it is :)

Review-Url: https://codereview.chromium.org/2868633002
Cr-Commit-Position: refs/heads/master@{#470354}
Committed: https://chromium.googlesource.com/chromium/src/+/08e198571dd5b5f51642bc5591277da924a42cab

[Ryan Hamilton, 2017-05-07 22:03:02]

Description was changed from

==========
Create a QuicChromiumClientStream::Handle class for allowing a stream
to a be accessed even after it's deleted. This will allow the upcalls
to be removed in subsequent CLs.
==========

to

==========
Create a QuicChromiumClientStream::Handle class for allowing a stream
to a be accessed even after it's deleted. This will allow the upcalls
to be removed in subsequent CLs.

This does NOT introduce any new semantics between the Bidi/Http stream and the
QUIC stream. It simply adds a layer of indirection between them. Subsequent CLs
will change semantics, switchings from upcalls to async IO. But this CL is big
enough as it is :)
==========

[Ryan Hamilton, 2017-05-07 22:39:49]

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

[Ryan Hamilton, 2017-05-07 22:39:49]

Here's the next CL in the pipeline :) It doesn't really *do* very much yet, but
it does introduce a Handel to the stream owned by the Bidi/Http stream which can
be used after the stream is dead. In followups, we'll actually do the hard work.

[Ryan Hamilton, 2017-05-08 14:03:30]

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

[commit-bot: I haz the power, 2017-05-08 14:03:50]

Dry run: CQ is trying da patch.

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

[Ryan Hamilton, 2017-05-08 14:20:43]

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

[commit-bot: I haz the power, 2017-05-08 14:20:52]

Dry run: 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, 2017-05-08 15:15:47]

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

[commit-bot: I haz the power, 2017-05-08 15:15:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xunjieli, 2017-05-08 19:51:01]

I am triage duty today, so here's a very brief round of review.

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_stream.cc (right):

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:120: //
nit: remove //

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:201: return 0;
0 is kV3HighestPriority. which can be misleading. Is there an int that means
invalid priority?  A consumer of Handle might call priority() once, saves that
value and never checks priority() again.

If this is a test-only method, I am fine with adding a "for_testing" suffix to
the method name or a DCHECK.

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:209: void
QuicChromiumClientStream::Handle::OnPromiseHeaderList(
nit: Can we match the order with the declaration in the header file?

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:220: void
QuicChromiumClientStream::Handle::SaveState() {
nit: Maybe add a DCHECK(stream_) here.

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_stream.h (right):

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.h:131: void
OnPromiseHeaderList(QuicStreamId promised_id,
This one doesn't seem to be test-only.

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.h:156: Delegate* delegate_;       
        // Owns this.
optional: I only see this commenting style in QUIC code. If possible, can we
standardize it so that the comment is placed above the member?

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
File net/quic/chromium/quic_http_stream_test.cc (right):

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream_test.cc:1531: //
reliable_stream->set_delegate(delegate);
nit: commented-out code.

[Ryan Hamilton, 2017-05-08 20:58:06]

Thanks for the comments while on triage!

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_stream.cc (right):

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:120: //
On 2017/05/08 19:51:00, xunjieli wrote:
> nit: remove //

Done.

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:201: return 0;
On 2017/05/08 19:51:00, xunjieli wrote:
> 0 is kV3HighestPriority. which can be misleading. Is there an int that means
> invalid priority?  A consumer of Handle might call priority() once, saves that
> value and never checks priority() again.
> 
> If this is a test-only method, I am fine with adding a "for_testing" suffix to
> the method name or a DCHECK.

Whoops, fixed. It's test-only and I'll eventually delete, but for now it works
correctly by saving priority().

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:209: void
QuicChromiumClientStream::Handle::OnPromiseHeaderList(
On 2017/05/08 19:51:00, xunjieli wrote:
> nit: Can we match the order with the declaration in the header file?

Done.

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:220: void
QuicChromiumClientStream::Handle::SaveState() {
On 2017/05/08 19:51:00, xunjieli wrote:
> nit: Maybe add a DCHECK(stream_) here.

Done.

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_stream.h (right):

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.h:131: void
OnPromiseHeaderList(QuicStreamId promised_id,
On 2017/05/08 19:51:00, xunjieli wrote:
> This one doesn't seem to be test-only.

I made it private, and the only error is from quic_http_stream_test.cc, so it
looks test-only to me. Do you see it used elsewhere?

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.h:156: Delegate* delegate_;       
        // Owns this.
On 2017/05/08 19:51:00, xunjieli wrote:
> optional: I only see this commenting style in QUIC code. If possible, can we
> standardize it so that the comment is placed above the member?

It appears to be used in other places in net too. For example:

https://cs.chromium.org/chromium/src/net/http/http_cache_transaction.h?rcl=53...

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
File net/quic/chromium/quic_http_stream_test.cc (right):

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream_test.cc:1531: //
reliable_stream->set_delegate(delegate);
On 2017/05/08 19:51:00, xunjieli wrote:
> nit: commented-out code.

Whoops. Good point. Removed.

[Ryan Hamilton, 2017-05-09 13:58:02]

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

[commit-bot: I haz the power, 2017-05-09 13:58:21]

Dry run: CQ is trying da patch.

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

[xunjieli, 2017-05-09 15:03:46]

looks great. LGTM. Sorry about the delay. I was trying to make sure I understand
the change :)

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_stream.h (right):

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.h:131: void
OnPromiseHeaderList(QuicStreamId promised_id,
On 2017/05/08 20:58:06, Ryan Hamilton wrote:
> On 2017/05/08 19:51:00, xunjieli wrote:
> > This one doesn't seem to be test-only.
> 
> I made it private, and the only error is from quic_http_stream_test.cc, so it
> looks test-only to me. Do you see it used elsewhere?

Acknowledged. I think I mistakenly assumed that this was the same one as the
Stream's OnPromiseHeaderList().

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.h:156: Delegate* delegate_;       
        // Owns this.
On 2017/05/08 20:58:05, Ryan Hamilton wrote:
> On 2017/05/08 19:51:00, xunjieli wrote:
> > optional: I only see this commenting style in QUIC code. If possible, can we
> > standardize it so that the comment is placed above the member?
> 
> It appears to be used in other places in net too. For example:
> 
>
https://cs.chromium.org/chromium/src/net/http/http_cache_transaction.h?rcl=53...

Acknowledged. Good point.

[commit-bot: I haz the power, 2017-05-09 15:51:47]

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

[commit-bot: I haz the power, 2017-05-09 15:51:48]

Dry run: This issue passed the CQ dry run.

[Ryan Hamilton, 2017-05-09 16:48:19]

Thanks so much!

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_stream.h (right):

https://codereview.chromium.org/2868633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.h:131: void
OnPromiseHeaderList(QuicStreamId promised_id,
On 2017/05/09 15:03:46, xunjieli wrote:
> On 2017/05/08 20:58:06, Ryan Hamilton wrote:
> > On 2017/05/08 19:51:00, xunjieli wrote:
> > > This one doesn't seem to be test-only.
> > 
> > I made it private, and the only error is from quic_http_stream_test.cc, so
it
> > looks test-only to me. Do you see it used elsewhere?
> 
> Acknowledged. I think I mistakenly assumed that this was the same one as the
> Stream's OnPromiseHeaderList().

I so know the feeling! I keep going back and forth between the HttpStream, the
Stream::Handle and the QuicStream. *head explodes*

[Ryan Hamilton, 2017-05-09 16:48:24]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2017-05-09 16:49:04]

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, 2017-05-09 16:56:58]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494348504627650,
"parent_rev": "4e18c94a1f6cc8f76720e1bf7b8f7b9cec1ad4ac", "commit_rev":
"08e198571dd5b5f51642bc5591277da924a42cab"}

[commit-bot: I haz the power, 2017-05-09 16:57:15]

Description was changed from

==========
Create a QuicChromiumClientStream::Handle class for allowing a stream
to a be accessed even after it's deleted. This will allow the upcalls
to be removed in subsequent CLs.

This does NOT introduce any new semantics between the Bidi/Http stream and the
QUIC stream. It simply adds a layer of indirection between them. Subsequent CLs
will change semantics, switchings from upcalls to async IO. But this CL is big
enough as it is :)
==========

to

==========
Create a QuicChromiumClientStream::Handle class for allowing a stream
to a be accessed even after it's deleted. This will allow the upcalls
to be removed in subsequent CLs.

This does NOT introduce any new semantics between the Bidi/Http stream and the
QUIC stream. It simply adds a layer of indirection between them. Subsequent CLs
will change semantics, switchings from upcalls to async IO. But this CL is big
enough as it is :)

Review-Url: https://codereview.chromium.org/2868633002
Cr-Commit-Position: refs/heads/master@{#470354}
Committed:
https://chromium.googlesource.com/chromium/src/+/08e198571dd5b5f51642bc559127...
==========

[commit-bot: I haz the power, 2017-05-09 16:57:16]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/08e198571dd5b5f51642bc559127...