SHP 4: Change AlternativeServiceMap to use SchemeHostPort as the key. No change to Pref data.

SHP 4: Change AlternativeServiceMap to use SchemeHostPort as the key. No change to Pref data.
Hardcoded to always add "https" as scheme when loading from Pref to Cache, always drop scheme when write to Pref from Cache.

Review only, should be landed in final unified CL.

BUG=600804

[Zhongyi Shi, 2016-04-14 03:52:33]

Description was changed from

==========
SHP 4(2): Change AlternativeServiceMap to use SchemeHostPort as the key. No
change to Pref data.
Hardcoded to always add "https" as scheme when loading from Pref to Cache,
always drop scheme when write to Pref from Cache.

Review only, should be landed in final unified CL.

BUG=600804
==========

to

==========
SHP 4: Change AlternativeServiceMap to use SchemeHostPort as the key. No change
to Pref data.
Hardcoded to always add "https" as scheme when loading from Pref to Cache,
always drop scheme when write to Pref from Cache.

Review only, should be landed in final unified CL.

BUG=600804
==========

[Zhongyi Shi, 2016-04-14 04:16:20]

Patchset #1 (id:1) has been deleted

[Zhongyi Shi, 2016-04-14 18:16:42]

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

[Zhongyi Shi, 2016-04-14 18:16:42]

Migrating Alt-Svc, woohoo!

[Ryan Hamilton, 2016-04-18 17:18:40]

lgtm

https://codereview.chromium.org/1878143005/diff/50001/net/http/http_server_pr...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/1878143005/diff/50001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:108: // Attempt to find canonical
servers. Canonical suffix only apply to HTTPS.
Does this represent a behavior change? If so, is this intentional?

https://codereview.chromium.org/1878143005/diff/50001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:109: uint16_t canonical_ports = 443;
Since this is a single port, you could say _port instead of _ports.
Possibly do const char* canonical_scheme = "https" to be consistent?

https://codereview.chromium.org/1878143005/diff/50001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:112: url::SchemeHostPort
canonical_host("https", canonical_suffix,
nit s/_host/_server/?

https://codereview.chromium.org/1878143005/diff/50001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:128: it->first.scheme() == "https") {
nit: instead of checking https here, you can use canonical_server.scheme

https://codereview.chromium.org/1878143005/diff/50001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:325: HostPortPair
host_port_pair(origin.host(), origin.port());
nit: move this down to where it's used.

https://codereview.chromium.org/1878143005/diff/50001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:342: // port, and both TCP), then there
is already a Job for it, so do not
nit: Can you change the comment to remove the mention of "then there is already
a Job for it"

https://codereview.chromium.org/1878143005/diff/50001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1878143005/diff/50001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1703: HostPortPair
host_port_pair(key_value.first.host(), key_value.first.port());
nit: move this down to where it's used (or perhaps just inline it into the
insert() call.

https://codereview.chromium.org/1878143005/diff/50001/net/spdy/spdy_network_t...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1878143005/diff/50001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:112: url::SchemeHostPort("http",
"www.example.org", 80),
https, perhaps?

[Ryan Hamilton, 2016-04-18 17:19:36]

Err, I mean, it's looking good but I have a question about the https-only
restriction. Did not mean to click L-G-T-M since it not lgtm yet.

Ugh. Mondays.

[Zhongyi Shi, 2016-04-18 19:26:37]

No worries, this is not the last CL and won't be used for landing. Plan to
generate final CL to land once everything is looking good. PTAL.

https://codereview.chromium.org/1878143005/diff/50001/net/http/http_server_pr...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/1878143005/diff/50001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:108: // Attempt to find canonical
servers. Canonical suffix only apply to HTTPS.
On 2016/04/18 17:18:40, Ryan Hamilton wrote:
> Does this represent a behavior change? If so, is this intentional?

According to our discussion
(https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...),
I think we agreed on apply canonical hosts to HTTPS only.

https://codereview.chromium.org/1878143005/diff/50001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:109: uint16_t canonical_ports = 443;
On 2016/04/18 17:18:40, Ryan Hamilton wrote:
> Since this is a single port, you could say _port instead of _ports.
> Possibly do const char* canonical_scheme = "https" to be consistent?

Done.

https://codereview.chromium.org/1878143005/diff/50001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:112: url::SchemeHostPort
canonical_host("https", canonical_suffix,
On 2016/04/18 17:18:40, Ryan Hamilton wrote:
> nit s/_host/_server/?

Done.

https://codereview.chromium.org/1878143005/diff/50001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:128: it->first.scheme() == "https") {
On 2016/04/18 17:18:40, Ryan Hamilton wrote:
> nit: instead of checking https here, you can use canonical_server.scheme

Done.

https://codereview.chromium.org/1878143005/diff/50001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:325: HostPortPair
host_port_pair(origin.host(), origin.port());
On 2016/04/18 17:18:40, Ryan Hamilton wrote:
> nit: move this down to where it's used.

Done.

https://codereview.chromium.org/1878143005/diff/50001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:342: // port, and both TCP), then there
is already a Job for it, so do not
On 2016/04/18 17:18:40, Ryan Hamilton wrote:
> nit: Can you change the comment to remove the mention of "then there is
already
> a Job for it"

Done.

https://codereview.chromium.org/1878143005/diff/50001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1878143005/diff/50001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1703: HostPortPair
host_port_pair(key_value.first.host(), key_value.first.port());
On 2016/04/18 17:18:40, Ryan Hamilton wrote:
> nit: move this down to where it's used (or perhaps just inline it into the
> insert() call.

It's used multiple times in the inner for loop. I feel better to declare it
here.

https://codereview.chromium.org/1878143005/diff/50001/net/spdy/spdy_network_t...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1878143005/diff/50001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:112: url::SchemeHostPort("http",
"www.example.org", 80),
On 2016/04/18 17:18:40, Ryan Hamilton wrote:
> https, perhaps?

Done.

[Ryan Hamilton, 2016-04-18 21:57:36]

lgtm thanks!

[Zhongyi Shi, 2016-04-19 19:08:18]

Fix SpdyNetworkTransactionTests and Cronet. PTAL :D

https://codereview.chromium.org/1878143005/diff/110001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.cc (right):

https://codereview.chromium.org/1878143005/diff/110001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:331: quic_hint.port());
Fixing cronet. Since it's quic_server, scheme here should be "https".

https://codereview.chromium.org/1878143005/diff/110001/net/spdy/spdy_network_...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1878143005/diff/110001/net/spdy/spdy_network_...
net/spdy/spdy_network_transaction_unittest.cc:113: url::SchemeHostPort("http",
"www.example.org", 80),
I should do this more carefully. This is the case where original URL is
http://www.example.org:80 per ssl_type == HTTP_SPDY_VIA_ALT_SVC. And thus we
should use "http" as the scheme.

[Ryan Hamilton, 2016-04-19 22:42:40]

lgtm