QUIC - use QuicSessionKey tuple (host, port, is_https) instead of server_hostname

QUIC - use QuicSessionKey tuple (host, port, is_https) instead of
server_hostname, while creating QuicClientSession,
QuicCryptoClientStream, QuicCryptoClientConfig, etc objects.

QuicSessionKey is used as the key to access QUIC server
config information from all caches (disk and memory caches).

On Disk cache, the key for accessing QUIC server information is the
flattened version (scheme://hostname:port) of QuicSession. 

scheme would be either http or https until we support other schemes.

R=rch@chromium.org, wtc@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257272

[ramant (doing other things), 2014-03-10 17:56:57]

Hi Ryan,
  Changed the disk caching of server config code to use port in addition to
hostname as the key.

  Will merged with your proxy changes after they land. Could you please take a
look at this change?

thanks
raman

[ramant (doing other things), 2014-03-10 20:03:23]

Hi Ryan and Wan-Teh,
  Sorry the previous upload failed. Re-uploaded again. Would appreciate your
opinion on ":" vs "/" as separator between hostname and port.

thanks
raman

[ramant (doing other things), 2014-03-10 20:31:46]

https://codereview.chromium.org/192583004/diff/60001/net/tools/quic/quic_clie...
File net/tools/quic/quic_client_session.cc (right):

https://codereview.chromium.org/192583004/diff/60001/net/tools/quic/quic_clie...
net/tools/quic/quic_client_session.cc:22: // Defaulting server_port to 9999.
We are not passing port for QuiClientSession and this code doesn't use chrome's
disk_cache (quic_server_info_factory is not initialized). Should we default to
port 0??

https://codereview.chromium.org/192583004/diff/60001/net/tools/quic/test_tool...
File net/tools/quic/test_tools/quic_test_client.cc (right):

https://codereview.chromium.org/192583004/diff/60001/net/tools/quic/test_tool...
net/tools/quic/test_tools/quic_test_client.cc:296:
config->LookupOrCreate(client_->server_hostname(), 9999);
We are not passing port for QuiTestClient and this code doesn't use chrome's
disk_cache (quic_server_info_factory is not initialized). Should we default to
port 0??

[Ryan Hamilton, 2014-03-10 20:57:33]

lgtm

Looks pretty resonable

https://codereview.chromium.org/192583004/diff/1/net/http/disk_cache_based_qu...
File net/http/disk_cache_based_quic_server_info.cc (right):

https://codereview.chromium.org/192583004/diff/1/net/http/disk_cache_based_qu...
net/http/disk_cache_based_quic_server_info.cc:119: return "quicserverinfo:" +
hostname_ + "/" + base::UintToString(port_);
nit: what do you think about ":" instead of "/"?  Probably doesn't matter *at*
*all*.

https://codereview.chromium.org/192583004/diff/1/net/http/disk_cache_based_qu...
File net/http/disk_cache_based_quic_server_info_unittest.cc (right):

https://codereview.chromium.org/192583004/diff/1/net/http/disk_cache_based_qu...
net/http/disk_cache_based_quic_server_info_unittest.cc:43: 443,
Should we have any test which demonstrates different info returned when the
ports differ?

https://codereview.chromium.org/192583004/diff/40001/net/quic/crypto/quic_ser...
File net/quic/crypto/quic_server_info.h (right):

https://codereview.chromium.org/192583004/diff/40001/net/quic/crypto/quic_ser...
net/quic/crypto/quic_server_info.h:27: QuicServerInfo(const std::string&
hostname, uint16 port);
nit: HostPortPair here and elsewhere? (excluding shared code)

https://codereview.chromium.org/192583004/diff/40001/net/quic/crypto/quic_ser...
net/quic/crypto/quic_server_info.h:107: virtual QuicServerInfo* GetForHost(const
std::string& hostname,
nit: GetForHostPort?

https://codereview.chromium.org/192583004/diff/40001/net/quic/test_tools/mock...
File net/quic/test_tools/mock_crypto_client_stream_factory.cc (right):

https://codereview.chromium.org/192583004/diff/40001/net/quic/test_tools/mock...
net/quic/test_tools/mock_crypto_client_stream_factory.cc:26: last_stream_ = new
MockCryptoClientStream(server_hostname, server_port,
nit: I think this wrapping is unusual.  I think that either server_hostname
should be on the next line, or else session should line up with server_hostname?

https://codereview.chromium.org/192583004/diff/40001/net/tools/quic/quic_clie...
File net/tools/quic/quic_client_session.cc (right):

https://codereview.chromium.org/192583004/diff/40001/net/tools/quic/quic_clie...
net/tools/quic/quic_client_session.cc:22: // Defaulting server_port to 9999.
Why 9999? Can we use the real port here instead?

[ramant (doing other things), 2014-03-11 00:48:49]

Hi Ryan,
  Made all the changes you have suggested and many thanks for your comments.
PTAL.

https://codereview.chromium.org/192583004/diff/1/net/http/disk_cache_based_qu...
File net/http/disk_cache_based_quic_server_info.cc (right):

https://codereview.chromium.org/192583004/diff/1/net/http/disk_cache_based_qu...
net/http/disk_cache_based_quic_server_info.cc:119: return "quicserverinfo:" +
hostname_ + "/" + base::UintToString(port_);
On 2014/03/10 20:57:33, Ryan Hamilton wrote:
> nit: what do you think about ":" instead of "/"?  Probably doesn't matter *at*
> *all*.

Done.

https://codereview.chromium.org/192583004/diff/1/net/http/disk_cache_based_qu...
File net/http/disk_cache_based_quic_server_info_unittest.cc (right):

https://codereview.chromium.org/192583004/diff/1/net/http/disk_cache_based_qu...
net/http/disk_cache_based_quic_server_info_unittest.cc:43: 443,
On 2014/03/10 20:57:33, Ryan Hamilton wrote:
> Should we have any test which demonstrates different info returned when the
> ports differ?

Done.

https://codereview.chromium.org/192583004/diff/40001/net/quic/crypto/quic_ser...
File net/quic/crypto/quic_server_info.h (right):

https://codereview.chromium.org/192583004/diff/40001/net/quic/crypto/quic_ser...
net/quic/crypto/quic_server_info.h:27: QuicServerInfo(const std::string&
hostname, uint16 port);
On 2014/03/10 20:57:33, Ryan Hamilton wrote:
> nit: HostPortPair here and elsewhere? (excluding shared code)

Changed everywhere except shared code (and/or files).

Done.

https://codereview.chromium.org/192583004/diff/40001/net/quic/crypto/quic_ser...
net/quic/crypto/quic_server_info.h:107: virtual QuicServerInfo* GetForHost(const
std::string& hostname,
On 2014/03/10 20:57:33, Ryan Hamilton wrote:
> nit: GetForHostPort?

Done.

https://codereview.chromium.org/192583004/diff/40001/net/quic/test_tools/mock...
File net/quic/test_tools/mock_crypto_client_stream_factory.cc (right):

https://codereview.chromium.org/192583004/diff/40001/net/quic/test_tools/mock...
net/quic/test_tools/mock_crypto_client_stream_factory.cc:26: last_stream_ = new
MockCryptoClientStream(server_hostname, server_port,
On 2014/03/10 20:57:33, Ryan Hamilton wrote:
> nit: I think this wrapping is unusual.  I think that either server_hostname
> should be on the next line, or else session should line up with
server_hostname?

Done.

https://codereview.chromium.org/192583004/diff/40001/net/tools/quic/quic_clie...
File net/tools/quic/quic_client_session.cc (right):

https://codereview.chromium.org/192583004/diff/40001/net/tools/quic/quic_clie...
net/tools/quic/quic_client_session.cc:22: // Defaulting server_port to 9999.
On 2014/03/10 20:57:33, Ryan Hamilton wrote:
> Why 9999? Can we use the real port here instead?

Thanks for the suggestion.

Done.

https://codereview.chromium.org/192583004/diff/60001/net/tools/quic/quic_clie...
File net/tools/quic/quic_client_session.cc (right):

https://codereview.chromium.org/192583004/diff/60001/net/tools/quic/quic_clie...
net/tools/quic/quic_client_session.cc:22: // Defaulting server_port to 9999.
On 2014/03/10 20:31:46, ramant wrote:
> We are not passing port for QuiClientSession and this code doesn't use
chrome's
> disk_cache (quic_server_info_factory is not initialized). Should we default to
> port 0??

Using the port from connection.

https://codereview.chromium.org/192583004/diff/60001/net/tools/quic/test_tool...
File net/tools/quic/test_tools/quic_test_client.cc (right):

https://codereview.chromium.org/192583004/diff/60001/net/tools/quic/test_tool...
net/tools/quic/test_tools/quic_test_client.cc:296:
config->LookupOrCreate(client_->server_hostname(), 9999);
On 2014/03/10 20:31:46, ramant wrote:
> We are not passing port for QuiTestClient and this code doesn't use chrome's
> disk_cache (quic_server_info_factory is not initialized). Should we default to
> port 0??

Using the port from client's server_address.

[Ryan Hamilton, 2014-03-11 00:55:49]

lgtm

[ramant (doing other things), 2014-03-11 01:10:27]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-03-11 01:14:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/192583004/100001

[wtc, 2014-03-11 01:41:39]

Patch set 5 LGTM.

I suggest not checking this in until we have resolved the issue of whether the
scheme (https:// or http://) part of the server URL should be part of the key.

High-level comments:

1. Please decide whether the scheme should be part of the key. I think it
should.

2. In disk_cache_based_quic_server_info_unittest.cc, I think you incorrectly
include "https://" in the hostname argument to the HostPortPair constructor.

3. In some functions you use a HostPortPair parameter, in others you use two
parameters -- a hostname and a port. This inconsistency should ideally be
eliminated.

Thanks.

https://codereview.chromium.org/192583004/diff/100001/net/http/disk_cache_bas...
File net/http/disk_cache_based_quic_server_info.cc (right):

https://codereview.chromium.org/192583004/diff/100001/net/http/disk_cache_bas...
net/http/disk_cache_based_quic_server_info.cc:118:
base::UintToString(host_port_pair_.port());

1. IMPORTANT: should the key include the scheme (https:// vs. http://)?

2. IMPORTANT: Can you use host_port_pair_.ToString()? This will add the square
brackets [] around the host part if the host part is an IPv6 address literal.

If you can use host_port_pair_.ToString(), please remember to remove #include
"base/strings/string_number_conversions.h".

https://codereview.chromium.org/192583004/diff/100001/net/http/disk_cache_bas...
File net/http/disk_cache_based_quic_server_info_unittest.cc (right):

https://codereview.chromium.org/192583004/diff/100001/net/http/disk_cache_bas...
net/http/disk_cache_based_quic_server_info_unittest.cc:57: net::HostPortPair
host_port_pair("https://www.verisign.com", 443);

IMPORTANT: the HostPortPair constructor you are using is:

  // If |in_host| represents an IPv6 address, it should not bracket the address.
  HostPortPair(const std::string& in_host, uint16 in_port);

So, I believe you incorrectly include the scheme ("https://") in the |in_host|
argument.

https://codereview.chromium.org/192583004/diff/100001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/192583004/diff/100001/net/http/http_cache.cc#...
net/http/http_cache.cc:278: virtual QuicServerInfo* GetForHostPortPair(

Nit: we can name this method "GetForServer". Then it's just a matter of how we
represent a server (by its hostname, by a host-port pair, or by
scheme://host:port).

https://codereview.chromium.org/192583004/diff/100001/net/quic/crypto/quic_cr...
File net/quic/crypto/quic_crypto_client_config.h (right):

https://codereview.chromium.org/192583004/diff/100001/net/quic/crypto/quic_cr...
net/quic/crypto/quic_crypto_client_config.h:151: uint16 port,

Can we replace the two argument with a HostPortPair argument?

Similarly for InitializeFrom.

https://codereview.chromium.org/192583004/diff/100001/net/quic/crypto/quic_se...
File net/quic/crypto/quic_server_info.h (right):

https://codereview.chromium.org/192583004/diff/100001/net/quic/crypto/quic_se...
net/quic/crypto/quic_server_info.h:107: virtual QuicServerInfo*
GetForHostPortPair(

Nit: consider naming this method "GetForServer".

https://codereview.chromium.org/192583004/diff/100001/net/quic/quic_client_se...
File net/quic/quic_client_session.h (right):

https://codereview.chromium.org/192583004/diff/100001/net/quic/quic_client_se...
net/quic/quic_client_session.h:95: uint16 server_port,

Should we use a HostPortPair argument here?

https://codereview.chromium.org/192583004/diff/100001/net/tools/quic/quic_cli...
File net/tools/quic/quic_client_session.cc (right):

https://codereview.chromium.org/192583004/diff/100001/net/tools/quic/quic_cli...
net/tools/quic/quic_client_session.cc:22: crypto_stream_(server_hostname,
connection->peer_address().port(), this,

IMPORTANT: it is strange that we get the hostname from a stand-alone argument,
but get the port from the |connection| argument.

[ramant (doing other things), 2014-03-11 01:44:09]

The CQ bit was unchecked by rtenneti@chromium.org

[wtc, 2014-03-11 15:37:52]

https://codereview.chromium.org/192583004/diff/100001/net/http/disk_cache_bas...
File net/http/disk_cache_based_quic_server_info.cc (right):

https://codereview.chromium.org/192583004/diff/100001/net/http/disk_cache_bas...
net/http/disk_cache_based_quic_server_info.cc:118:
base::UintToString(host_port_pair_.port());

On 2014/03/11 01:41:39, wtc wrote:
> 
> 1. IMPORTANT: should the key include the scheme ( https:// vs. http:// )?

Raman, Ryan: I think the key should include the scheme. If it is hard to know
what the scheme is, I think it is sufficient to indicate whether the QUIC server
crypto config comes with a proof, in other words, whether it is secure QUIC or
insecure QUIC. QuicCryptoClientStream should have this info. We can encode this
one bit of info in the key. For example, if there is a proof, we can add a "s/"
prefix to the host:port pair, such as "s/www.google.com:443".

[Ryan Hamilton, 2014-03-11 15:42:41]

On 2014/03/11 15:37:52, wtc wrote:
>
https://codereview.chromium.org/192583004/diff/100001/net/http/disk_cache_bas...
> File net/http/disk_cache_based_quic_server_info.cc (right):
> 
>
https://codereview.chromium.org/192583004/diff/100001/net/http/disk_cache_bas...
> net/http/disk_cache_based_quic_server_info.cc:118:
> base::UintToString(host_port_pair_.port());
> 
> On 2014/03/11 01:41:39, wtc wrote:
> > 
> > 1. IMPORTANT: should the key include the scheme ( https:// vs. http:// )?
> 
> Raman, Ryan: I think the key should include the scheme. If it is hard to know
> what the scheme is, I think it is sufficient to indicate whether the QUIC
server
> crypto config comes with a proof, in other words, whether it is secure QUIC or
> insecure QUIC. QuicCryptoClientStream should have this info. We can encode
this
> one bit of info in the key. For example, if there is a proof, we can add a
"s/"
> prefix to the host:port pair, such as "s/www.google.com:443".

Including the scheme seems fine to me.  I suspect that not including the scheme
will likely not lead to any security problems, but after my alternate-protocol
screw up, I'm happy to be safe, just in case.  I think we should always be able
to know if the connection is secure or not (http vs https) since that's
explicitly passed in to the stream factory request. Personally, I would be
inclined to make the key <scheme>://<host>:<port> since that looks like a URL,
but your proposed s/ is also perfectly sensible.

[Ryan Hamilton, 2014-03-11 15:47:43]

https://codereview.chromium.org/192583004/diff/100001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/192583004/diff/100001/net/http/http_cache.cc#...
net/http/http_cache.cc:278: virtual QuicServerInfo* GetForHostPortPair(
On 2014/03/11 01:41:39, wtc wrote:
> 
> Nit: we can name this method "GetForServer". Then it's just a matter of how we
> represent a server (by its hostname, by a host-port pair, or by
> scheme://host:port).

Nice!  Or, if we're going to include scheme, we could also consider origin.

https://codereview.chromium.org/192583004/diff/100001/net/quic/crypto/quic_cr...
File net/quic/crypto/quic_crypto_client_config.h (right):

https://codereview.chromium.org/192583004/diff/100001/net/quic/crypto/quic_cr...
net/quic/crypto/quic_crypto_client_config.h:151: uint16 port,
On 2014/03/11 01:41:39, wtc wrote:
> 
> Can we replace the two argument with a HostPortPair argument?
> 
> Similarly for InitializeFrom.

As I understand it, HostPortPair is a chrome-ism which does not exist internally
so we decided to use the 2 arg route for shared code.  That being said, we could
easily port HostPortPair to the internal codebase.  Perhaps that would be
cleaner.  What do you guys this?

https://codereview.chromium.org/192583004/diff/100001/net/tools/quic/quic_cli...
File net/tools/quic/quic_client_session.cc (right):

https://codereview.chromium.org/192583004/diff/100001/net/tools/quic/quic_cli...
net/tools/quic/quic_client_session.cc:22: crypto_stream_(server_hostname,
connection->peer_address().port(), this,
On 2014/03/11 01:41:39, wtc wrote:
> 
> IMPORTANT: it is strange that we get the hostname from a stand-alone argument,
> but get the port from the |connection| argument.

It looks like QuicStreamFactory has added port to the QuicClientSession
constructor, but here in QuicClientSession I don't see the new argument.

[wtc, 2014-03-11 15:50:19]

https://codereview.chromium.org/192583004/diff/100001/net/http/disk_cache_bas...
File net/http/disk_cache_based_quic_server_info.cc (right):

https://codereview.chromium.org/192583004/diff/100001/net/http/disk_cache_bas...
net/http/disk_cache_based_quic_server_info.cc:118:
base::UintToString(host_port_pair_.port());

Ryan, thanks for the comment.

If we can get the scheme, I prefer the scheme over my "s/" prefix. The "s/"
suggestion should only be considered if we cannot get the scheme.

I also want to clarify that we should avoid inferring the scheme ("https" or
"http") based on whether the connection is secure or not, because QUIC could
also be used by non-HTTP protocols.

[ramant (doing other things), 2014-03-13 01:50:11]

Hi Ryan and Wan-Teh,
  Thanks for your comments. Made the changes we had dicussed offline. Use
QuicSessionKey everywhere to access all caches.

  Used "server_key" as the variable name instead of session_key.

  Will land this change after the following CL lands and after merging with it.

 https://codereview.chromium.org/196403002/

thanks
raman

https://codereview.chromium.org/192583004/diff/100001/net/http/disk_cache_bas...
File net/http/disk_cache_based_quic_server_info.cc (right):

https://codereview.chromium.org/192583004/diff/100001/net/http/disk_cache_bas...
net/http/disk_cache_based_quic_server_info.cc:118:
base::UintToString(host_port_pair_.port());
On 2014/03/11 01:41:39, wtc wrote:
> 
> 1. IMPORTANT: should the key include the scheme (https:// vs. http://%29?
> 
> 2. IMPORTANT: Can you use host_port_pair_.ToString()? This will add the square
> brackets [] around the host part if the host part is an IPv6 address literal.
> 
> If you can use host_port_pair_.ToString(), please remember to remove #include
> "base/strings/string_number_conversions.h".

Called ToString method on quic_session_key. Currently it supports http/https
only. we persist <scheme>://<host>:port as the key.

Done.

https://codereview.chromium.org/192583004/diff/100001/net/http/disk_cache_bas...
net/http/disk_cache_based_quic_server_info.cc:118:
base::UintToString(host_port_pair_.port());
On 2014/03/11 15:37:52, wtc wrote:
> 
> On 2014/03/11 01:41:39, wtc wrote:
> > 
> > 1. IMPORTANT: should the key include the scheme ( https:// vs. http:// )?
> 
> Raman, Ryan: I think the key should include the scheme. If it is hard to know
> what the scheme is, I think it is sufficient to indicate whether the QUIC
server
> crypto config comes with a proof, in other words, whether it is secure QUIC or
> insecure QUIC. QuicCryptoClientStream should have this info. We can encode
this
> one bit of info in the key. For example, if there is a proof, we can add a
"s/"
> prefix to the host:port pair, such as "s/www.google.com:443".

Using  <scheme>://<host>:port as the key. scheme for now is either "http" or
"https".

https://codereview.chromium.org/192583004/diff/100001/net/http/disk_cache_bas...
net/http/disk_cache_based_quic_server_info.cc:118:
base::UintToString(host_port_pair_.port());
On 2014/03/11 01:41:39, wtc wrote:
> 
> 1. IMPORTANT: should the key include the scheme (https:// vs. http://%29?
> 
> 2. IMPORTANT: Can you use host_port_pair_.ToString()? This will add the square
> brackets [] around the host part if the host part is an IPv6 address literal.
> 
> If you can use host_port_pair_.ToString(), please remember to remove #include
> "base/strings/string_number_conversions.h".

Done.

https://codereview.chromium.org/192583004/diff/100001/net/http/disk_cache_bas...
File net/http/disk_cache_based_quic_server_info_unittest.cc (right):

https://codereview.chromium.org/192583004/diff/100001/net/http/disk_cache_bas...
net/http/disk_cache_based_quic_server_info_unittest.cc:57: net::HostPortPair
host_port_pair("https://www.verisign.com", 443);
On 2014/03/11 01:41:39, wtc wrote:
> 
> IMPORTANT: the HostPortPair constructor you are using is:
> 
>   // If |in_host| represents an IPv6 address, it should not bracket the
address.
>   HostPortPair(const std::string& in_host, uint16 in_port);
> 
> So, I believe you incorrectly include the scheme ("https://") in the |in_host|
> argument.

Done.

https://codereview.chromium.org/192583004/diff/100001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/192583004/diff/100001/net/http/http_cache.cc#...
net/http/http_cache.cc:278: virtual QuicServerInfo* GetForHostPortPair(
On 2014/03/11 01:41:39, wtc wrote:
> 
> Nit: we can name this method "GetForServer". Then it's just a matter of how we
> represent a server (by its hostname, by a host-port pair, or by
> scheme://host:port).

Done.

https://codereview.chromium.org/192583004/diff/100001/net/http/http_cache.cc#...
net/http/http_cache.cc:278: virtual QuicServerInfo* GetForHostPortPair(
On 2014/03/11 15:47:43, Ryan Hamilton wrote:
> On 2014/03/11 01:41:39, wtc wrote:
> > 
> > Nit: we can name this method "GetForServer". Then it's just a matter of how
we
> > represent a server (by its hostname, by a host-port pair, or by
> > scheme://host:port).
> 
> Nice!  Or, if we're going to include scheme, we could also consider origin.

Acknowledged. Used GetServer per wtc's suggestion.

https://codereview.chromium.org/192583004/diff/100001/net/quic/crypto/quic_cr...
File net/quic/crypto/quic_crypto_client_config.h (right):

https://codereview.chromium.org/192583004/diff/100001/net/quic/crypto/quic_cr...
net/quic/crypto/quic_crypto_client_config.h:151: uint16 port,
On 2014/03/11 15:47:43, Ryan Hamilton wrote:
> On 2014/03/11 01:41:39, wtc wrote:
> > 
> > Can we replace the two argument with a HostPortPair argument?
> > 
> > Similarly for InitializeFrom.
> 
> As I understand it, HostPortPair is a chrome-ism which does not exist
internally
> so we decided to use the 2 arg route for shared code.  That being said, we
could
> easily port HostPortPair to the internal codebase.  Perhaps that would be
> cleaner.  What do you guys this?

Used QuicSessionKey and will port HostPortPair to shared code.

https://codereview.chromium.org/192583004/diff/100001/net/quic/crypto/quic_cr...
net/quic/crypto/quic_crypto_client_config.h:151: uint16 port,
On 2014/03/11 15:47:43, Ryan Hamilton wrote:
> On 2014/03/11 01:41:39, wtc wrote:
> > 
> > Can we replace the two argument with a HostPortPair argument?
> > 
> > Similarly for InitializeFrom.
> 
> As I understand it, HostPortPair is a chrome-ism which does not exist
internally
> so we decided to use the 2 arg route for shared code.  That being said, we
could
> easily port HostPortPair to the internal codebase.  Perhaps that would be
> cleaner.  What do you guys this?

Will port HostPortPair as we merge this CL into shared code.

https://codereview.chromium.org/192583004/diff/100001/net/quic/crypto/quic_se...
File net/quic/crypto/quic_server_info.h (right):

https://codereview.chromium.org/192583004/diff/100001/net/quic/crypto/quic_se...
net/quic/crypto/quic_server_info.h:107: virtual QuicServerInfo*
GetForHostPortPair(
On 2014/03/11 01:41:39, wtc wrote:
> 
> Nit: consider naming this method "GetForServer".

Done.

https://codereview.chromium.org/192583004/diff/100001/net/quic/quic_client_se...
File net/quic/quic_client_session.h (right):

https://codereview.chromium.org/192583004/diff/100001/net/quic/quic_client_se...
net/quic/quic_client_session.h:95: uint16 server_port,
On 2014/03/11 01:41:39, wtc wrote:
> 
> Should we use a HostPortPair argument here?

Used QuicSessionKey.

https://codereview.chromium.org/192583004/diff/100001/net/tools/quic/quic_cli...
File net/tools/quic/quic_client_session.cc (right):

https://codereview.chromium.org/192583004/diff/100001/net/tools/quic/quic_cli...
net/tools/quic/quic_client_session.cc:22: crypto_stream_(server_hostname,
connection->peer_address().port(), this,
On 2014/03/11 15:47:43, Ryan Hamilton wrote:
> On 2014/03/11 01:41:39, wtc wrote:
> > 
> > IMPORTANT: it is strange that we get the hostname from a stand-alone
argument,
> > but get the port from the |connection| argument.
> 
> It looks like QuicStreamFactory has added port to the QuicClientSession
> constructor, but here in QuicClientSession I don't see the new argument.

Passed QuicSessionKey as argument to QuicClientSession and QuicClient.

https://codereview.chromium.org/192583004/diff/100001/net/tools/quic/quic_cli...
net/tools/quic/quic_client_session.cc:22: crypto_stream_(server_hostname,
connection->peer_address().port(), this,
On 2014/03/11 01:41:39, wtc wrote:
> 
> IMPORTANT: it is strange that we get the hostname from a stand-alone argument,
> but get the port from the |connection| argument.

Added QuicSessionKey as argument to QuicClientSession and QuicClient (leaving it
to the callers to specify QuicSessionKey).

[ramant (doing other things), 2014-03-13 15:53:45]

Hi Ryan and Wan-Teh,
  Was thinking whereever we are passing server_hostname and server_key, would
like to pass only server_key. If that sounds ok, will make that change,

[Ryan Hamilton, 2014-03-13 16:53:03]

On 2014/03/13 15:53:45, ramant wrote:
> Hi Ryan and Wan-Teh,
>   Was thinking whereever we are passing server_hostname and server_key, would
> like to pass only server_key. If that sounds ok, will make that change,

SGTM

[ramant (doing other things), 2014-03-13 20:34:18]

Hi Ryan and Wan-Teh,
  Deleted server_hostname as argument. Would appreciate a high level review.
Some of this code needs to be moved after rch's changes land and get merged.

Thanks

https://codereview.chromium.org/192583004/diff/270001/net/tools/quic/quic_cli...
File net/tools/quic/quic_client_bin.cc (right):

https://codereview.chromium.org/192583004/diff/270001/net/tools/quic/quic_cli...
net/tools/quic/quic_client_bin.cc:45: "--secure                    check
certificates\n";
not sure if this is right name/description. Wanted to indicate if secure is
true, we will create ProofVerfier and ProofVerifier will verify the
certificates.

[Ryan Hamilton, 2014-03-13 22:14:39]

LGTM.  I think you might want to update the CL description since the scope of
the CL is not specifically tied to persisting crypto config.

https://codereview.chromium.org/192583004/diff/270001/net/tools/quic/quic_cli...
File net/tools/quic/quic_client_bin.cc (right):

https://codereview.chromium.org/192583004/diff/270001/net/tools/quic/quic_cli...
net/tools/quic/quic_client_bin.cc:45: "--secure                    check
certificates\n";
On 2014/03/13 20:34:19, ramant wrote:
> not sure if this is right name/description. Wanted to indicate if secure is
> true, we will create ProofVerfier and ProofVerifier will verify the
> certificates.

SGTM.

[wtc, 2014-03-13 22:22:03]

Patch set 6 LGTM.

IMPORTANT: in places where the original parameter or class member name is
"server_hostname", replacing that name with "session_key" makes the code less
readable.

I think the new name must retain the "server_" part. We can try "server_key",
"server_session_key", or even "server_origin" (because QuicSessionKey contains
the same info as an "origin").

https://codereview.chromium.org/192583004/diff/270001/net/quic/crypto/quic_se...
File net/quic/crypto/quic_server_info.h (right):

https://codereview.chromium.org/192583004/diff/270001/net/quic/crypto/quic_se...
net/quic/crypto/quic_server_info.h:96: // This is the QUIC server (is_https,
hostname, port) tuple for which we

Nit: in the QuicSessionKey constructors, is_https is the last parameter. So it
seems better to also list is_https last here.

https://codereview.chromium.org/192583004/diff/270001/net/quic/quic_client_se...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/192583004/diff/270001/net/quic/quic_client_se...
net/quic/quic_client_session.cc:117: NetLog::StringCallback("host",
&session_key.host_port_pair().host()));

Nit: perhaps QuicSessionKey should provide a host() getter method to provide
direct access to the hostname.

https://codereview.chromium.org/192583004/diff/270001/net/quic/quic_crypto_cl...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/192583004/diff/270001/net/quic/quic_crypto_cl...
net/quic/quic_crypto_client_stream.cc:70: const QuicSessionKey& session_key,

Nit: this parameter probably should be named |server_key| to be more
descriptive.

https://codereview.chromium.org/192583004/diff/270001/net/quic/quic_crypto_cl...
File net/quic/quic_crypto_client_stream.h (right):

https://codereview.chromium.org/192583004/diff/270001/net/quic/quic_crypto_cl...
net/quic/quic_crypto_client_stream.h:116: // Server's (is_https, hostname, port)
tuple.

Nit: same here -- might be better to list is_https last to match the order of
QuicSessionKey constructor parameters.

[ramant (doing other things), 2014-03-13 23:46:35]

Used server_key whereever old variables server_hostname (or hostname) are used.
quic_stream_factory.cc is the only file where we were using session_key variable
before and left that variable name as is.

https://codereview.chromium.org/192583004/diff/270001/net/quic/crypto/quic_se...
File net/quic/crypto/quic_server_info.h (right):

https://codereview.chromium.org/192583004/diff/270001/net/quic/crypto/quic_se...
net/quic/crypto/quic_server_info.h:96: // This is the QUIC server (is_https,
hostname, port) tuple for which we
On 2014/03/13 22:22:03, wtc wrote:
> 
> Nit: in the QuicSessionKey constructors, is_https is the last parameter. So it
> seems better to also list is_https last here.

Done.

https://codereview.chromium.org/192583004/diff/270001/net/quic/quic_client_se...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/192583004/diff/270001/net/quic/quic_client_se...
net/quic/quic_client_session.cc:117: NetLog::StringCallback("host",
&session_key.host_port_pair().host()));
On 2014/03/13 22:22:03, wtc wrote:
> 
> Nit: perhaps QuicSessionKey should provide a host() getter method to provide
> direct access to the hostname.

Done.

https://codereview.chromium.org/192583004/diff/270001/net/quic/quic_crypto_cl...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/192583004/diff/270001/net/quic/quic_crypto_cl...
net/quic/quic_crypto_client_stream.cc:70: const QuicSessionKey& session_key,
On 2014/03/13 22:22:03, wtc wrote:
> 
> Nit: this parameter probably should be named |server_key| to be more
> descriptive.

Done.

https://codereview.chromium.org/192583004/diff/270001/net/quic/quic_crypto_cl...
File net/quic/quic_crypto_client_stream.h (right):

https://codereview.chromium.org/192583004/diff/270001/net/quic/quic_crypto_cl...
net/quic/quic_crypto_client_stream.h:116: // Server's (is_https, hostname, port)
tuple.
On 2014/03/13 22:22:03, wtc wrote:
> 
> Nit: same here -- might be better to list is_https last to match the order of
> QuicSessionKey constructor parameters.

Done.

[ramant (doing other things), 2014-03-14 19:14:07]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-03-14 19:14:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/192583004/310001

[commit-bot: I haz the power, 2014-03-14 20:11:13]

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

[commit-bot: I haz the power, 2014-03-14 20:11:14]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[ramant (doing other things), 2014-03-14 21:32:19]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-03-14 21:36:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/192583004/310001

[commit-bot: I haz the power, 2014-03-15 00:45:16]

Change committed as 257272

[wtc, 2014-03-17 17:13:20]

Patch set 8 LGTM.

https://codereview.chromium.org/192583004/diff/310001/net/quic/quic_client_se...
File net/quic/quic_client_session.h (right):

https://codereview.chromium.org/192583004/diff/310001/net/quic/quic_client_se...
net/quic/quic_client_session.h:96: const QuicSessionKey& server_key,

Nit: server_info and server_key probably should be listed next to each other.

https://codereview.chromium.org/192583004/diff/310001/net/quic/quic_session_k...
File net/quic/quic_session_key.h (right):

https://codereview.chromium.org/192583004/diff/310001/net/quic/quic_session_k...
net/quic/quic_session_key.h:31: const HostPortPair& host_port_pair() const {

Nit: Please see if we can get rid of the host_port_pair() getter method.

[ramant (doing other things), 2014-03-17 18:27:35]

https://codereview.chromium.org/192583004/diff/310001/net/quic/quic_client_se...
File net/quic/quic_client_session.h (right):

https://codereview.chromium.org/192583004/diff/310001/net/quic/quic_client_se...
net/quic/quic_client_session.h:96: const QuicSessionKey& server_key,
On 2014/03/17 17:13:21, wtc wrote:
> 
> Nit: server_info and server_key probably should be listed next to each other.

Fixed it in https://codereview.chromium.org/202143004

Done.

https://codereview.chromium.org/192583004/diff/310001/net/quic/quic_session_k...
File net/quic/quic_session_key.h (right):

https://codereview.chromium.org/192583004/diff/310001/net/quic/quic_session_k...
net/quic/quic_session_key.h:31: const HostPortPair& host_port_pair() const {
On 2014/03/17 17:13:21, wtc wrote:
> 
> Nit: Please see if we can get rid of the host_port_pair() getter method.

It is being used by quic_stream_factory. Will leave it if you don't have
objections (else we need to create HostPortPair object from QuicSessionKey).

It uses host_port_pair for
"HostResolver::RequestInfo(session_key_.host_port_pair())" and to call factory's
CreateSession

[wtc, 2014-03-18 23:50:12]

https://codereview.chromium.org/192583004/diff/310001/net/quic/quic_session_k...
File net/quic/quic_session_key.h (right):

https://codereview.chromium.org/192583004/diff/310001/net/quic/quic_session_k...
net/quic/quic_session_key.h:31: const HostPortPair& host_port_pair() const {

On 2014/03/17 18:27:36, ramant wrote:
>
> [The host_port_pair() getter method] is being used by quic_stream_factory.
Will leave it if you don't have
> objections (else we need to create HostPortPair object from QuicSessionKey).
> 
> It uses host_port_pair for
> "HostResolver::RequestInfo(session_key_.host_port_pair())" and to call
factory's
> CreateSession

Thanks for the reply. Please keep the getter method then.