Change the AlternativeServiceMap with SchemeOriginPair key.

Change the AlternativeServiceMap with SchemeOriginPair key. No behavior change.

BUG=595938

[Zhongyi Shi, 2016-03-22 00:52:54]

Description was changed from

==========
Alt-Svc 6: Change ClearAlternativeServices to use SchemeOriginPair
Alt-Svc 5: Change SetAlternativeService(s) to use SchemeOriginPair
Alt-Svc 3: Change SupportsRequestPriority to take SchemeOriginPair arg in the
virtual func
Change the interface of GetAlternativeServices in impl, and manager

BUG=595938
==========

to

==========
Change the AlternativeServiceMap with SchemeOriginPair key. No behavior change.

BUG=595938
==========

[Zhongyi Shi, 2016-03-22 20:44:01]

Patchset #2 (id:20001) has been deleted

[Zhongyi Shi, 2016-03-22 20:49:07]

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

[Zhongyi Shi, 2016-03-22 20:49:08]

This cl still uses newly defined SchemeOriginPair. But it should be fine to use
the SchemeHostPort class, probably just need adding some methods to that class.
PTAL, thanks!

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:563: SchemeOriginPair
scheme_origin_pair("https", server);
I'm not sure, whether setting the scheme to be "https" is safe here. Or do we
need the info from callers?

[Ryan Hamilton, 2016-03-24 22:23:14]

Looking pretty good. I think there are a few more places where we should use
this new SHP object. (And I think we should use the version in url::)

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties.h:210: class NET_EXPORT SchemeOriginPair {
I think we should definitely use url::SchemeHostPort unless there is some reason
that doesn't work.

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties.h:288: virtual bool GetSupportsSpdy(const
HostPortPair& server) = 0;
Should these next 6 methods also use a SHP?

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

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:117: "https",
canonical_host_to_origin_map_[canonical_host])) !=
If we want canoncical suffixes to only apply to HTTPS (which seems reasonable)
it'd be good to add a comment to that effect. Should also remove port 80 on line
109 (and avoid the loop).

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:351: if (origin.scheme() == "https" &&
I don't understand this check. Can you say more?

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:368: if (origin.scheme() != "https")
Comments.

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:377: SchemeOriginPair("https",
canonical->second));
can you use origin.scheme() instead?

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:463: if (origin.scheme() == "https" &&
comments.

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:532: if (origin.scheme() == "https")
comments.

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
File net/http/http_server_properties_impl.h (right):

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_impl.h:181: // actual origin, which has a
plausible alternate protocol mapping.
It seems like the key should really be a scheme/host/port now that we're doing
that instead of host/port.

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:563: SchemeOriginPair
scheme_origin_pair("https", server);
On 2016/03/22 20:49:08, Zhongyi Shi wrote:
> I'm not sure, whether setting the scheme to be "https" is safe here. Or do we
> need the info from callers? 

I recommend chatting with Raman about this. He's the expert at the serialization
layer and has done this kind of change before.

[Zhongyi Shi, 2016-03-29 01:00:45]

Patchset #5 (id:100001) has been deleted

[Zhongyi Shi, 2016-04-04 18:58:34]

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

[Zhongyi Shi, 2016-04-04 18:58:36]

I will build another CL which updates disk data depending on this CL since we
need part of this CL's changes in that one.

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties.h:210: class NET_EXPORT SchemeOriginPair {
On 2016/03/24 22:23:14, Ryan Hamilton wrote:
> I think we should definitely use url::SchemeHostPort unless there is some
reason
> that doesn't work.

Done.

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties.h:288: virtual bool GetSupportsSpdy(const
HostPortPair& server) = 0;
On 2016/03/24 22:23:14, Ryan Hamilton wrote:
> Should these next 6 methods also use a SHP?

I don't think so, only SupportsRequestPriority queries the Alt-Svc map and the
other methods doesn't refer to that map.

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

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:117: "https",
canonical_host_to_origin_map_[canonical_host])) !=
On 2016/03/24 22:23:14, Ryan Hamilton wrote:
> If we want canoncical suffixes to only apply to HTTPS (which seems reasonable)
> it'd be good to add a comment to that effect. Should also remove port 80 on
line
> 109 (and avoid the loop).

Done.

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:351: if (origin.scheme() == "https" &&
On 2016/03/24 22:23:14, Ryan Hamilton wrote:
> I don't understand this check. Can you say more?

I thought we want to check whether the alt-svc is equivalent to origin. But, I
think you're right, we are actually checking whether there's a job already
running. So no need to check the origin's scheme.

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:368: if (origin.scheme() != "https")
On 2016/03/24 22:23:14, Ryan Hamilton wrote:
> Comments.
Changing the CanonicalHostMap to be <SHP, SHP>, no need to check origin's scheme
anymore :D

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:377: SchemeOriginPair("https",
canonical->second));
On 2016/03/24 22:23:14, Ryan Hamilton wrote:
> can you use origin.scheme() instead?

Acknowledged.

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:463: if (origin.scheme() == "https" &&
On 2016/03/24 22:23:14, Ryan Hamilton wrote:
> comments.

Done.

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:532: if (origin.scheme() == "https")
On 2016/03/24 22:23:14, Ryan Hamilton wrote:
> comments.

Acknowledged.

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
File net/http/http_server_properties_impl.h (right):

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_impl.h:181: // actual origin, which has a
plausible alternate protocol mapping.
On 2016/03/24 22:23:14, Ryan Hamilton wrote:
> It seems like the key should really be a scheme/host/port now that we're doing
> that instead of host/port.

Done.

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:563: SchemeOriginPair
scheme_origin_pair("https", server);
On 2016/03/24 22:23:14, Ryan Hamilton wrote:
> On 2016/03/22 20:49:08, Zhongyi Shi wrote:
> > I'm not sure, whether setting the scheme to be "https" is safe here. Or do
we
> > need the info from callers? 
> 
> I recommend chatting with Raman about this. He's the expert at the
serialization
> layer and has done this kind of change before.

After chatting with Raman and a deep digging in the code, found a lot more
changes to persist disk data. We need to change all the keys from HostPortPair
to SchemeHostPort, now seperate code to new dependent CL.

[Ryan Hamilton, 2016-04-06 19:11:37]

rch@chromium.org changed reviewers:
- rch@chromium.org

[Zhongyi Shi, 2016-04-06 23:47:56]

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