Server push cancellation: add methods in session layer to cancel push given the pushed url. Not in …

Server push cancellation: add methods in session layer to cancel push given the pushed url. Not in use.

BUG=232040
Committed: https://crrev.com/18bb2d951716466214da9a99a85ad48c7aa4cfa8
Cr-Commit-Position: refs/heads/master@{#428114}

[Zhongyi Shi, 2016-10-27 08:48:43]

Description was changed from

==========
Server push cancellation: add methods in session layer to cancel push given the
pushed url. Not in use.

BUG=232040
==========

to

==========
Server push cancellation: add methods in session layer to cancel push given the
pushed url. Not in use.

BUG=232040
==========

[Zhongyi Shi, 2016-10-27 08:48:43]

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

[Ryan Hamilton, 2016-10-27 17:00:25]

https://codereview.chromium.org/2456713002/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2456713002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:1453:
DeletePromised(promised_info);
I'm a bit surprised to see the delete before the reset, but I guess that works?
Does the other order work?

https://codereview.chromium.org/2456713002/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_session.h (right):

https://codereview.chromium.org/2456713002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.h:308: void CancelPush(const
GURL& url) override;
nit: please comment, although I imagine this is pretty obvious. (What happens if
this is called for a URL that a push is not pending for)

https://codereview.chromium.org/2456713002/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_session_test.cc (right):

https://codereview.chromium.org/2456713002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session_test.cc:314:
TEST_P(QuicChromiumClientSessionTest, CancelPushBeforeReceiveResponse) {
nit: s/Receive/Receiving/

https://codereview.chromium.org/2456713002/diff/40001/net/quic/core/quic_clie...
File net/quic/core/quic_client_session_base.h (right):

https://codereview.chromium.org/2456713002/diff/40001/net/quic/core/quic_clie...
net/quic/core/quic_client_session_base.h:115: virtual void CancelPush(const
GURL& url);
I don't think I understand why we need this method here since the .cc file has a
no-op implementation. I'm guessing this is some sort of polymorphism mechanism?

[Zhongyi Shi, 2016-10-27 17:57:30]

Thanks Ryan, comments addressed, PTAL!

https://codereview.chromium.org/2456713002/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2456713002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:1453:
DeletePromised(promised_info);
On 2016/10/27 17:00:25, Ryan Hamilton wrote:
> I'm a bit surprised to see the delete before the reset, but I guess that
works?
> Does the other order work?

The other order also works. DeletePromised() deletes metadata for push promise
from push_promise_index, it doesn't affect the stream layer. But I'll reorder
the calls if it literally makes more sense.

https://codereview.chromium.org/2456713002/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_session.h (right):

https://codereview.chromium.org/2456713002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.h:308: void CancelPush(const
GURL& url) override;
On 2016/10/27 17:00:25, Ryan Hamilton wrote:
> nit: please comment, although I imagine this is pretty obvious. (What happens
if
> this is called for a URL that a push is not pending for)

Done.

https://codereview.chromium.org/2456713002/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_session_test.cc (right):

https://codereview.chromium.org/2456713002/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session_test.cc:314:
TEST_P(QuicChromiumClientSessionTest, CancelPushBeforeReceiveResponse) {
On 2016/10/27 17:00:25, Ryan Hamilton wrote:
> nit: s/Receive/Receiving/

Done.

https://codereview.chromium.org/2456713002/diff/40001/net/quic/core/quic_clie...
File net/quic/core/quic_client_session_base.h (right):

https://codereview.chromium.org/2456713002/diff/40001/net/quic/core/quic_clie...
net/quic/core/quic_client_session_base.h:115: virtual void CancelPush(const
GURL& url);
On 2016/10/27 17:00:25, Ryan Hamilton wrote:
> I don't think I understand why we need this method here since the .cc file has
a
> no-op implementation. I'm guessing this is some sort of polymorphism
mechanism?

You are right. I don't need this anymore. I was thinking to make this function
more general so that we could also call it when push times out (which then
requires QuicClientSessionBase to have the method). Then I gave up... So yeah,
removing it now. Thanks for catching it!

[Ryan Hamilton, 2016-10-27 18:21:00]

lgtm

[Zhongyi Shi, 2016-10-27 18:39:14]

zhongyi@chromium.org changed reviewers:
- ckrasic@chromium.org

[Zhongyi Shi, 2016-10-27 18:39:21]

The CQ bit was checked by zhongyi@chromium.org

[commit-bot: I haz the power, 2016-10-27 18:40:07]

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-10-27 19:39:11]

Description was changed from

==========
Server push cancellation: add methods in session layer to cancel push given the
pushed url. Not in use.

BUG=232040
==========

to

==========
Server push cancellation: add methods in session layer to cancel push given the
pushed url. Not in use.

BUG=232040
==========

[commit-bot: I haz the power, 2016-10-27 19:39:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-27 20:00:12]

Description was changed from

==========
Server push cancellation: add methods in session layer to cancel push given the
pushed url. Not in use.

BUG=232040
==========

to

==========
Server push cancellation: add methods in session layer to cancel push given the
pushed url. Not in use.

BUG=232040

Committed: https://crrev.com/18bb2d951716466214da9a99a85ad48c7aa4cfa8
Cr-Commit-Position: refs/heads/master@{#428114}
==========

[commit-bot: I haz the power, 2016-10-27 20:00:13]

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