[Chromecast] Enable QUIC for Chromecast.

[Chromecast] Enable QUIC for Chromecast.

Also allows QUIC to be disabled at runtime if needed. To re-enable QUIC,
the device (or at least, cast_shell) must be restarted.

BUG= internal b/32742342
TEST= cast Youtube, verify there are QUIC packets in the tcpdump.

Review-Url: https://codereview.chromium.org/2656283002
Cr-Commit-Position: refs/heads/master@{#447024}
Committed: https://chromium.googlesource.com/chromium/src/+/58fd01a42d6d2d82a21534e9d4e2270ff3c3584f

[mbjorge, 2017-01-27 01:44:36]

mbjorge@chromium.org changed reviewers:
+ halliwell@chromium.org, lcwu@chromium.org

[mbjorge, 2017-01-27 01:44:36]

Two main items in this CL:

1. DisableQuic() method in URLRequestContextFactory that handles disabling Quic
for all the URLRequestContexts

2. Passing a closure down so that cast_shell can disable quic at runtime. In
this CL, the closure is created in CastBrowserMainParts which passes ownership
through the CastService. The closure will take one more hop in the internal CL.

[mbjorge, 2017-01-27 02:01:19]

mbjorge@chromium.org changed reviewers:
+ maclellant@chromium.org

[halliwell, 2017-01-27 04:32:53]

Headed to the internal part of the review next ... :)

https://codereview.chromium.org/2656283002/diff/1/chromecast/browser/cast_con...
File chromecast/browser/cast_content_browser_client.h (right):

https://codereview.chromium.org/2656283002/diff/1/chromecast/browser/cast_con...
chromecast/browser/cast_content_browser_client.h:56: using DisableQuicClosure =
base::OnceClosure;
include base/callback_forward.h

https://codereview.chromium.org/2656283002/diff/1/chromecast/browser/url_requ...
File chromecast/browser/url_request_context_factory.cc (right):

https://codereview.chromium.org/2656283002/diff/1/chromecast/browser/url_requ...
chromecast/browser/url_request_context_factory.cc:323: LOG(INFO) << "Set
HttpNetworkSessionParams.enable_quic = " << enable_quic_;
how frequently does this get logged?  Just want to make sure it's not going to
be spammy.

https://codereview.chromium.org/2656283002/diff/1/chromecast/browser/url_requ...
chromecast/browser/url_request_context_factory.cc:438: base::Unretained(this)));
Should this be done with weak ptr, or is there a guarantee that this object
lives until posted task is run?

https://codereview.chromium.org/2656283002/diff/1/chromecast/browser/url_requ...
chromecast/browser/url_request_context_factory.cc:448: // Disable QUIC for any
new URLContextGetter that gets created.
this and the next 3 comments don't add value (what the code does is
self-evident)

[mbjorge, 2017-01-27 06:22:20]

https://codereview.chromium.org/2656283002/diff/1/chromecast/browser/cast_con...
File chromecast/browser/cast_content_browser_client.h (right):

https://codereview.chromium.org/2656283002/diff/1/chromecast/browser/cast_con...
chromecast/browser/cast_content_browser_client.h:56: using DisableQuicClosure =
base::OnceClosure;
On 2017/01/27 at 04:32:53, halliwell wrote:
> include base/callback_forward.h

done

https://codereview.chromium.org/2656283002/diff/1/chromecast/browser/url_requ...
File chromecast/browser/url_request_context_factory.cc (right):

https://codereview.chromium.org/2656283002/diff/1/chromecast/browser/url_requ...
chromecast/browser/url_request_context_factory.cc:323: LOG(INFO) << "Set
HttpNetworkSessionParams.enable_quic = " << enable_quic_;
On 2017/01/27 at 04:32:53, halliwell wrote:
> how frequently does this get logged?  Just want to make sure it's not going to
be spammy.

It'll get logged a max of 3 times, once for each of the
Main/System/MediaContextGetters when they get created.

https://codereview.chromium.org/2656283002/diff/1/chromecast/browser/url_requ...
chromecast/browser/url_request_context_factory.cc:438: base::Unretained(this)));
On 2017/01/27 at 04:32:53, halliwell wrote:
> Should this be done with weak ptr, or is there a guarantee that this object
lives until posted task is run?

URLRequestContextFactory is owned by CastContentBrowserClient, so it will live
as long as that does, which to my understanding was the entire life of
cast_shell, since it's getting created/owned by CastMainDelegate in
cast_main.cc, so it should outlive basically everything.

I don't see harm in adding it though, so I'm happy to add it if it makes it more
obviously safe to a reader. I've tried just adding a comment for now describing
why it should be safe.

https://codereview.chromium.org/2656283002/diff/1/chromecast/browser/url_requ...
chromecast/browser/url_request_context_factory.cc:448: // Disable QUIC for any
new URLContextGetter that gets created.
On 2017/01/27 at 04:32:53, halliwell wrote:
> this and the next 3 comments don't add value (what the code does is
self-evident)

done

[halliwell, 2017-01-27 18:35:35]

lgtm

https://codereview.chromium.org/2656283002/diff/1/chromecast/browser/url_requ...
File chromecast/browser/url_request_context_factory.cc (right):

https://codereview.chromium.org/2656283002/diff/1/chromecast/browser/url_requ...
chromecast/browser/url_request_context_factory.cc:438: base::Unretained(this)));
On 2017/01/27 06:22:20, mbjorge wrote:
> On 2017/01/27 at 04:32:53, halliwell wrote:
> > Should this be done with weak ptr, or is there a guarantee that this object
> lives until posted task is run?
> 
> URLRequestContextFactory is owned by CastContentBrowserClient, so it will live
> as long as that does, which to my understanding was the entire life of
> cast_shell, since it's getting created/owned by CastMainDelegate in
> cast_main.cc, so it should outlive basically everything.
> 
> I don't see harm in adding it though, so I'm happy to add it if it makes it
more
> obviously safe to a reader. I've tried just adding a comment for now
describing
> why it should be safe. 

That's fine, looks good.

[maclellant, 2017-01-30 18:41:09]

lgtm

[mbjorge, 2017-01-30 18:41:31]

The CQ bit was checked by mbjorge@chromium.org

[commit-bot: I haz the power, 2017-01-30 18:41:56]

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-01-30 19:04:11]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485801691030030,
"parent_rev": "ee25559e75c9fa03648b4fcaf52f162baa243d2a", "commit_rev":
"58fd01a42d6d2d82a21534e9d4e2270ff3c3584f"}

[commit-bot: I haz the power, 2017-01-30 19:04:41]

Description was changed from

==========
[Chromecast] Enable QUIC for Chromecast.

Also allows QUIC to be disabled at runtime if needed. To re-enable QUIC,
the device (or at least, cast_shell) must be restarted.

BUG= internal b/32742342
TEST= cast Youtube, verify there are QUIC packets in the tcpdump.
==========

to

==========
[Chromecast] Enable QUIC for Chromecast.

Also allows QUIC to be disabled at runtime if needed. To re-enable QUIC,
the device (or at least, cast_shell) must be restarted.

BUG= internal b/32742342
TEST= cast Youtube, verify there are QUIC packets in the tcpdump.

Review-Url: https://codereview.chromium.org/2656283002
Cr-Commit-Position: refs/heads/master@{#447024}
Committed:
https://chromium.googlesource.com/chromium/src/+/58fd01a42d6d2d82a21534e9d4e2...
==========

[commit-bot: I haz the power, 2017-01-30 19:04:48]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/58fd01a42d6d2d82a21534e9d4e2...