Cancel a promised stream if it is for a request with a body in QuicHttpStream.

Cancel a promised stream if it is for a request with a body in QuicHttpStream.

BUG=

Committed: https://crrev.com/e3e96efd76a0241fc4db3e450c0a77553e0ec403
Cr-Commit-Position: refs/heads/master@{#418944}

[Ryan Hamilton, 2016-09-15 03:45:27]

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

[Ryan Hamilton, 2016-09-15 03:45:28]

One more minor QuicHttpStream cleanup..

[Buck, 2016-09-15 17:13:58]

lgtm

https://codereview.chromium.org/2345663002/diff/1/net/quic/chromium/quic_http...
File net/quic/chromium/quic_http_stream.cc (right):

https://codereview.chromium.org/2345663002/diff/1/net/quic/chromium/quic_http...
net/quic/chromium/quic_http_stream.cc:240: // A request with a body is ineligble
for push, so reset the
nit: s/ineligble/ineligible/

https://codereview.chromium.org/2345663002/diff/1/net/quic/chromium/quic_http...
net/quic/chromium/quic_http_stream.cc:251: 
totally up to you, but I feel like this would be nicer if it just did
push_handle_->Cancel()

Of course, you don't have push_handle_ yet, but maybe something like this
refactor:

  o rename TryHandle to PushHandle
      o in addition to Cancel(), it also has Try()
Refactor the current QuicClientPushPromiseIndex::Try() so into two bits:
  o new method QuicClientPushPromiseIndex::Lookup() (returns PushHandle)
           o using this, found_promised_ could be consolidated into push_handle_
  o remainder of Try() logic goes into new PushHandle::Try() method

...or I could do all that in a followup CL.

[Buck, 2016-09-15 17:13:59]

lgtm

[Ryan Hamilton, 2016-09-15 19:09:06]

Thanks!

https://codereview.chromium.org/2345663002/diff/1/net/quic/chromium/quic_http...
File net/quic/chromium/quic_http_stream.cc (right):

https://codereview.chromium.org/2345663002/diff/1/net/quic/chromium/quic_http...
net/quic/chromium/quic_http_stream.cc:240: // A request with a body is ineligble
for push, so reset the
On 2016/09/15 17:13:58, Buck wrote:
> nit: s/ineligble/ineligible/

Done.

https://codereview.chromium.org/2345663002/diff/1/net/quic/chromium/quic_http...
net/quic/chromium/quic_http_stream.cc:251: 
On 2016/09/15 17:13:58, Buck wrote:
> totally up to you, but I feel like this would be nicer if it just did
> push_handle_->Cancel()

Agreed! I was actually expecting to be able to do that. But as you observe
below, we don't yet have the handle.

> Of course, you don't have push_handle_ yet, but maybe something like this
> refactor:
> 
>   o rename TryHandle to PushHandle
>       o in addition to Cancel(), it also has Try()
> Refactor the current QuicClientPushPromiseIndex::Try() so into two bits:
>   o new method QuicClientPushPromiseIndex::Lookup() (returns PushHandle)
>            o using this, found_promised_ could be consolidated into
push_handle_
>   o remainder of Try() logic goes into new PushHandle::Try() method
> 
> ...or I could do all that in a followup CL.

Sounds like a nice refactor. If you'd like to do it, that sounds great to me!

[Ryan Hamilton, 2016-09-15 19:09:09]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2016-09-15 19:09:10]

The patchset sent to the CQ was uploaded after l-g-t-m from ckrasic@chromium.org
Link to the patchset: https://codereview.chromium.org/2345663002/#ps20001
(title: "fix comment")

[commit-bot: I haz the power, 2016-09-15 19:09:55]

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, 2016-09-15 20:04:53]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-15 20:07:53]

Description was changed from

==========
Cancel a promised stream if it is for a request with a body in QuicHttpStream.

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

to

==========
Cancel a promised stream if it is for a request with a body in QuicHttpStream.

BUG=

Committed: https://crrev.com/e3e96efd76a0241fc4db3e450c0a77553e0ec403
Cr-Commit-Position: refs/heads/master@{#418944}
==========

[commit-bot: I haz the power, 2016-09-15 20:07:53]

Patchset 2 (id:??) landed as
https://crrev.com/e3e96efd76a0241fc4db3e450c0a77553e0ec403
Cr-Commit-Position: refs/heads/master@{#418944}