SHP 1: Change SupportsSpdy dict to use SchemeHostPort as the key. No change to Pref data.

SHP 1: Change SupportsSpdy dict to use SchemeHostPort as the key. No change to Pref data.
Hardcoded to always add "http" as scheme when loading from Pref to Cache, always drop scheme when write to Pref from Cache.

BUG=600804

[Zhongyi Shi, 2016-04-06 00:32:09]

Patchset #1 (id:1) has been deleted

[Zhongyi Shi, 2016-04-06 00:34:48]

zhongyi@chromium.org changed reviewers:
+ rch@chromium.org, rtenneti@google.com

[Ryan Hamilton, 2016-04-06 19:16:08]

Looking good.

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

https://codereview.chromium.org/1860343002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:267:
SpdyServerSchemeHostPortMap::iterator spdy_scheme_host_port =
how about spdy_server?

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

https://codereview.chromium.org/1860343002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties_impl.h:140: typedef base::MRUCache<std::string,
bool> SpdyServerSchemeHostPortMap;
How about just SpdyServerMap?

https://codereview.chromium.org/1860343002/diff/20001/net/http/http_server_pr...
File net/http/http_server_properties_impl_unittest.cc (right):

https://codereview.chromium.org/1860343002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties_impl_unittest.cc:115: url::SchemeHostPort
spdy_server_mail("https", "mail.google.com", 443);
We should have tests which verify that the same host:port but different schemes
are treated differently.

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

https://codereview.chromium.org/1860343002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:573: std::string
server_scheme_host_port = "http://" + server_str;
Hm. Seems like we should convert servers_dict at the same time that we convert
the map.

https://codereview.chromium.org/1860343002/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1860343002/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.cc:1430: host_port_pair.port());
I think we need to fix this now. What would it take to use the correct scheme?

[Zhongyi Shi, 2016-04-07 00:31:17]

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

[Zhongyi Shi, 2016-04-07 00:31:18]

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

https://codereview.chromium.org/1860343002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:267:
SpdyServerSchemeHostPortMap::iterator spdy_scheme_host_port =
On 2016/04/06 19:16:08, Ryan Hamilton wrote:
> how about spdy_server?

Done.

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

https://codereview.chromium.org/1860343002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties_impl.h:140: typedef base::MRUCache<std::string,
bool> SpdyServerSchemeHostPortMap;
On 2016/04/06 19:16:08, Ryan Hamilton wrote:
> How about just SpdyServerMap?

Done.

https://codereview.chromium.org/1860343002/diff/20001/net/http/http_server_pr...
File net/http/http_server_properties_impl_unittest.cc (right):

https://codereview.chromium.org/1860343002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties_impl_unittest.cc:115: url::SchemeHostPort
spdy_server_mail("https", "mail.google.com", 443);
On 2016/04/06 19:16:08, Ryan Hamilton wrote:
> We should have tests which verify that the same host:port but different
schemes
> are treated differently.

Done.

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

https://codereview.chromium.org/1860343002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:573: std::string
server_scheme_host_port = "http://" + server_str;
On 2016/04/06 19:16:08, Ryan Hamilton wrote:
> Hm. Seems like we should convert servers_dict at the same time that we convert
> the map.

Will do when we migrating servers_dict!:D

https://codereview.chromium.org/1860343002/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1860343002/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.cc:1430: host_port_pair.port());
On 2016/04/06 19:16:08, Ryan Hamilton wrote:
> I think we need to fix this now. What would it take to use the correct scheme?

Done.

[Zhongyi Shi, 2016-04-07 18:13:37]

Patchset #4 (id:80001) has been deleted

[Ryan Hamilton, 2016-04-07 18:27:40]

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

https://codereview.chromium.org/1860343002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_impl_unittest.cc:107: // Check spdy servers are
correctly sett with SchemeHostPort key.
s/sett/set/

https://codereview.chromium.org/1860343002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_impl_unittest.cc:117: url::SchemeHostPort
valid_google_server((GURL("https://www.google.com")));
nit: there are 5 variables here which are all SHP, but the names could be a bit
cleaner, I think. 

Instead of:
 spdy_server_google cs http_google_server and
 spdy_server_photots cs https_photos_server, how about:

http_www_server
https_www_server
http_photos_server
https_photos_server

https://codereview.chromium.org/1860343002/diff/60001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1860343002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.cc:1428: bool is_https =
spdy_session_key.proxy_server().is_https();
This doesn't seem right since you're checking the scheme of the proxy server,
not a property of the spdy session. Can you check GetSSLInfo() on the session
directly?

[Zhongyi Shi, 2016-04-07 21:15:01]

Patchset #5 (id:120001) has been deleted

[Zhongyi Shi, 2016-04-07 21:19:34]

Addressing comments, fixed scheme extraction in HttpStreamFactoryImplJob. PTAL.

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

https://codereview.chromium.org/1860343002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_impl_unittest.cc:107: // Check spdy servers are
correctly sett with SchemeHostPort key.
On 2016/04/07 18:27:40, Ryan Hamilton wrote:
> s/sett/set/

Done.

https://codereview.chromium.org/1860343002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_impl_unittest.cc:117: url::SchemeHostPort
valid_google_server((GURL("https://www.google.com")));
On 2016/04/07 18:27:40, Ryan Hamilton wrote:
> nit: there are 5 variables here which are all SHP, but the names could be a
bit
> cleaner, I think. 
> 
> Instead of:
>  spdy_server_google cs http_google_server and
>  spdy_server_photots cs https_photos_server, how about:
> 
> http_www_server
> https_www_server
> http_photos_server
> https_photos_server

Done.

https://codereview.chromium.org/1860343002/diff/60001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1860343002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.cc:1428: bool is_https =
spdy_session_key.proxy_server().is_https();
On 2016/04/07 18:27:40, Ryan Hamilton wrote:
> This doesn't seem right since you're checking the scheme of the proxy server,
> not a property of the spdy session. Can you check GetSSLInfo() on the session
> directly?

Sorry for miss that. Using SSL info on the session now.

[Ryan Hamilton, 2016-04-08 18:32:51]

looks good though as discussed, we won't land this

https://codereview.chromium.org/1860343002/diff/140001/net/http/http_server_p...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/1860343002/diff/140001/net/http/http_server_p...
net/http/http_server_properties_manager.cc:574: std::string spdy_server_url =
"http://" + server_str;
I know this is temporary, but might as well make this https:// for now :>

[Zhongyi Shi, 2016-04-08 21:22:10]

Patchset #6 (id:160001) has been deleted

[Zhongyi Shi, 2016-04-11 22:45:17]

Patchset #8 (id:220001) has been deleted