Server push cancellation: change the ownership of ServerPushDelegate to HttpNetworkSession.

Server push cancellation: add a setter method for server push delegate in HttpNetworkSession. Plumb the server push delegate to QuicStreamFactory/SpdySessionPool so that QuicChromiumClientSession/SpdySession takes server push delegate in contructor.

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

[Zhongyi Shi, 2016-11-23 02:29:37]

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

[Ryan Hamilton, 2016-11-23 04:22:41]

I think the CL description is a bit off. It doesn't seem like it's *moving*
ownership of the ServerPushDelegate so much as it is making the
QuicStreamFactory and SpdySessionPooling take the delegate via a setter instead
of a constructor argument, right? Is it required that these setters be called
before using the factor/pool? What about the network session?

looking good, just some questions.

https://codereview.chromium.org/2521573006/diff/1/net/http/http_network_sessi...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/2521573006/diff/1/net/http/http_network_sessi...
net/http/http_network_session.h:274: // |quic_stream_factory_| and
|spdy_session_pool_|.
No reason to mention ownership, since the argument is a unique_ptr.

[Zhongyi Shi, 2016-11-23 06:10:54]

Description was changed from

==========
Server push cancellation: change the ownership of ServerPushDelegate to
HttpNetworkSession.

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

to

==========
Server push cancellation: add a setter method for server push delegate in
HttpNetworkSession. Plumb the server push delegate to
QuicStreamFactory/SpdySessionPool so that QuicChromiumClientSession/SpdySession
takes server push delegate in contructor.


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

[Zhongyi Shi, 2016-11-23 06:19:37]

On 2016/11/23 04:22:41, Ryan Hamilton wrote:
> I think the CL description is a bit off. It doesn't seem like it's *moving*
> ownership of the ServerPushDelegate so much as it is making the
> QuicStreamFactory and SpdySessionPooling take the delegate via a setter
instead
> of a constructor argument, right? Is it required that these setters be called
> before using the factor/pool? What about the network session?

Just updated the cl description. It's actually change the
QuicChromiumClientSession/SpdySession to
take the server push delegate via constuctor. But make
QuicStreamFactory/SpdySessionPool to take the
server push delegate via setter method(Factory/Pool used to not own the ref to
push delegate). 

The network session will own a server push delegate when HttpCache is
constructed from a HttpNetworkSession
https://cs.chromium.org/chromium/src/net/http/http_cache.cc?sq=package:chromi...
It will be similar to how we construct the QuicServerInfoFactoryAdaptor. I am
leaving that part in the cl
which set up the finch parameter.

[Zhongyi Shi, 2016-11-23 06:20:02]

https://codereview.chromium.org/2521573006/diff/1/net/http/http_network_sessi...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/2521573006/diff/1/net/http/http_network_sessi...
net/http/http_network_session.h:274: // |quic_stream_factory_| and
|spdy_session_pool_|.
On 2016/11/23 04:22:41, Ryan Hamilton wrote:
> No reason to mention ownership, since the argument is a unique_ptr.

Done.

[Buck, 2016-11-23 20:18:36]

lgtm

[Ryan Hamilton, 2016-11-23 20:43:30]

two small questions.

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

https://codereview.chromium.org/2521573006/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.h:134: ServerPushDelegate*
push_delegate,
Should this method DCHECK that push_delegate is non-null?

https://codereview.chromium.org/2521573006/diff/20001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/2521573006/diff/20001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:666: push_delegate_(push_delegate),
ditto on the non-null check question.

[Zhongyi Shi, 2016-11-23 23:53:49]

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

https://codereview.chromium.org/2521573006/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.h:134: ServerPushDelegate*
push_delegate,
On 2016/11/23 20:43:30, Ryan Hamilton wrote:
> Should this method DCHECK that push_delegate is non-null?

Nope. The push_delegate will be null if we don't enable server push
cancellation.

https://codereview.chromium.org/2521573006/diff/20001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/2521573006/diff/20001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:666: push_delegate_(push_delegate),
On 2016/11/23 20:43:30, Ryan Hamilton wrote:
> ditto on the non-null check question.

Acknowledged.

[Ryan Hamilton, 2016-11-24 00:38:23]

lgtm

Seems like we should have a test in which SetServerPushDelegate() is called to
pass in a server delegate?

[Ryan Hamilton, 2016-12-12 22:54:45]

On 2016/11/24 00:38:23, Ryan Hamilton wrote:
> lgtm
> 
> Seems like we should have a test in which SetServerPushDelegate() is called to
> pass in a server delegate?

Is this still active?

[Zhongyi Shi, 2016-12-12 22:58:45]

On 2016/12/12 22:54:45, Ryan Hamilton wrote:
> On 2016/11/24 00:38:23, Ryan Hamilton wrote:
> > lgtm
> > 
> > Seems like we should have a test in which SetServerPushDelegate() is called
to
> > pass in a server delegate?
> 
> Is this still active?

Yup. I'll update this CL once merge rotation finished.

[Zhongyi Shi, 2016-12-15 00:57:17]

On 2016/12/12 22:54:45, Ryan Hamilton wrote:
> On 2016/11/24 00:38:23, Ryan Hamilton wrote:
> > lgtm
> > 
> > Seems like we should have a test in which SetServerPushDelegate() is called
to
> > pass in a server delegate?
> 
> Is this still active?

Ryan, sorry for getting back to this late. AFAIK, we don't have
HttpNetworkSession unittests. Could you say more what we should test for
SetServerPushDelegate?
Are you concerned about whether the server push delegate will get successfully
plumbed to the SpdySessionPool and QuicStreamFactory? If so, we could possibly
add simple tests
in quic_network_transaction_unittests to see whether the quic_stream_factory
gets the server push delegate when session sets up the push delegate.

[Ryan Hamilton, 2016-12-15 01:32:09]

On 2016/12/15 00:57:17, Zhongyi Shi wrote:
> On 2016/12/12 22:54:45, Ryan Hamilton wrote:
> > On 2016/11/24 00:38:23, Ryan Hamilton wrote:
> > > lgtm
> > > 
> > > Seems like we should have a test in which SetServerPushDelegate() is
called
> to
> > > pass in a server delegate?
> > 
> > Is this still active?
> 
> Ryan, sorry for getting back to this late. AFAIK, we don't have
> HttpNetworkSession unittests. Could you say more what we should test for
> SetServerPushDelegate?
> Are you concerned about whether the server push delegate will get successfully
> plumbed to the SpdySessionPool and QuicStreamFactory? If so, we could possibly
> add simple tests
> in quic_network_transaction_unittests to see whether the quic_stream_factory
> gets the server push delegate when session sets up the push delegate.

I think we have url_request_quic_unittest.cc. Would that work?

[Zhongyi Shi, 2016-12-19 02:20:23]

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

[commit-bot: I haz the power, 2016-12-19 02:20:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhongyi Shi, 2016-12-19 03:06:53]

Had an offline discussion with rch@, I'll land this CL as is and regression test
will be in the follow-up cl which enables the finch trial. Thank you for helping
the review.

[commit-bot: I haz the power, 2016-12-19 03:30:20]

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

[commit-bot: I haz the power, 2016-12-19 03:30:21]

Dry run: This issue passed the CQ dry run.

[Zhongyi Shi, 2016-12-19 03:31:00]

The CQ bit was checked by zhongyi@chromium.org

[Zhongyi Shi, 2016-12-19 03:31:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
ckrasic@chromium.org, rch@chromium.org
Link to the patchset: https://codereview.chromium.org/2521573006/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-19 03:31:21]

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-12-19 03:36:04]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482118260565920,
"parent_rev": "8a65c5f06e92046124da9a20e83d452451e38c6d", "commit_rev":
"6ea4a67eca49b1fee343acec002b8cb0235bf665"}

[commit-bot: I haz the power, 2016-12-19 03:36:31]

Description was changed from

==========
Server push cancellation: add a setter method for server push delegate in
HttpNetworkSession. Plumb the server push delegate to
QuicStreamFactory/SpdySessionPool so that QuicChromiumClientSession/SpdySession
takes server push delegate in contructor.


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

to

==========
Server push cancellation: add a setter method for server push delegate in
HttpNetworkSession. Plumb the server push delegate to
QuicStreamFactory/SpdySessionPool so that QuicChromiumClientSession/SpdySession
takes server push delegate in contructor.


BUG=232040

Review-Url: https://codereview.chromium.org/2521573006
==========

[commit-bot: I haz the power, 2016-12-19 03:36:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-19 03:38:53]

Description was changed from

==========
Server push cancellation: add a setter method for server push delegate in
HttpNetworkSession. Plumb the server push delegate to
QuicStreamFactory/SpdySessionPool so that QuicChromiumClientSession/SpdySession
takes server push delegate in contructor.


BUG=232040

Review-Url: https://codereview.chromium.org/2521573006
==========

to

==========
Server push cancellation: add a setter method for server push delegate in
HttpNetworkSession. Plumb the server push delegate to
QuicStreamFactory/SpdySessionPool so that QuicChromiumClientSession/SpdySession
takes server push delegate in contructor.

BUG=232040

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

[commit-bot: I haz the power, 2016-12-19 03:38:55]

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