Move QuicServerInfo management from CachedState to the QuicStreamFactory

Move QuicServerInfo management from CachedState to the QuicStreamFactory
and QuicCryptoClientStream.

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

[Ryan Hamilton, 2014-03-11 23:56:52]

I think this is an improvement.  What do you think?  The next step will be to
break QuicCryptoClientConfig into a base class and a chromium specific class.

[ramant (doing other things), 2014-03-12 00:50:55]

lgtm

https://codereview.chromium.org/196403002/diff/1/net/quic/quic_crypto_client_...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/196403002/diff/1/net/quic/quic_crypto_client_...
net/quic/quic_crypto_client_stream.cc:16: #include
"net/quic/crypto/quic_server_info.h"
nit: line 16 could be deleted.

https://codereview.chromium.org/196403002/diff/1/net/quic/quic_crypto_client_...
net/quic/quic_crypto_client_stream.cc:280: cached->SetProofValid();
nit: consider calling SaveQuicServerInfo from here? 

nit: minor optimization - should we avoid persisting the config to disk if the
config we have loaded from disk cache is valid and didn't change.

https://codereview.chromium.org/196403002/diff/1/net/quic/quic_stream_factory.cc
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/196403002/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.cc:175: io_state_ = STATE_RESOLVE_HOST;
nit: should we initialize the state to STATE_LOAD_SERVER_INFO?

[Ryan Hamilton, 2014-03-12 04:00:44]

Thanks!

https://codereview.chromium.org/196403002/diff/1/net/quic/quic_crypto_client_...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/196403002/diff/1/net/quic/quic_crypto_client_...
net/quic/quic_crypto_client_stream.cc:16: #include
"net/quic/crypto/quic_server_info.h"
On 2014/03/12 00:50:56, ramant wrote:
> nit: line 16 could be deleted.

Done.

https://codereview.chromium.org/196403002/diff/1/net/quic/quic_crypto_client_...
net/quic/quic_crypto_client_stream.cc:280: cached->SetProofValid();
On 2014/03/12 00:50:56, ramant wrote:
> nit: consider calling SaveQuicServerInfo from here? 

Done, and in the other places.  We should probably have test coverage of this
path.

> nit: minor optimization - should we avoid persisting the config to disk if the
> config we have loaded from disk cache is valid and didn't change.

Good idea.  I'll look into that!

https://codereview.chromium.org/196403002/diff/1/net/quic/quic_stream_factory.cc
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/196403002/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.cc:175: io_state_ = STATE_RESOLVE_HOST;
On 2014/03/12 00:50:56, ramant wrote:
> nit: should we initialize the state to STATE_LOAD_SERVER_INFO?

Ugh!  Yes, you're right.  I need to write some tests :>

[Ryan Hamilton, 2014-03-12 19:00:46]

wtc: Can you take a look?  I think this is all sane, but I'd love it if you
could provide a sanity check.

[wtc, 2014-03-12 23:13:20]

Patch set 4 LGTM.

https://codereview.chromium.org/196403002/diff/60001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_client_config.h (right):

https://codereview.chromium.org/196403002/diff/60001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.h:79: bool
Initialize(base::StringPiece server_config,

Nit: if the Initialize method is similar to InitializeFrom, I suggest listing
them next to each other.

Please document what the bool return value means. Since InitializeFrom returns
void, I was surprised to see Initialize return bool.

Also, I am wondering why Initialize needs a |QuicWallTime now| input, but
InitializeFrom doesn't.

https://codereview.chromium.org/196403002/diff/60001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.h:82: const std::vector<std::string>&
certs,

I suggest listing the first four arguments in the same order as the
corresponding class members: server_config, source_address_token, certs,
signature.

If you reorder them, please be careful because three of the arguments have the
same type, so the compiler cannot distinguish between them if you make a
mistake.

https://codereview.chromium.org/196403002/diff/60001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.h (right):

https://codereview.chromium.org/196403002/diff/60001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.h:108: void
SaveQuicServerInfo(QuicCryptoClientConfig::CachedState* cached);

1. Please document that we should call SaveQuicServerInfo whenever we call
cached->SetProofValid().

Perhaps this should be done by cached->SetProofValid()?

2. |cached| should be a const pointer or const reference.

https://codereview.chromium.org/196403002/diff/60001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.h:138: scoped_ptr<QuicServerInfo>
quic_server_info_;

Nit: because of the "The result of certificate verification." comment, it would
be nice to add a blank line to make it clear this is not part of that group.

[Ryan Hamilton, 2014-03-13 16:38:07]

Thanks!

https://codereview.chromium.org/196403002/diff/60001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_client_config.h (right):

https://codereview.chromium.org/196403002/diff/60001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.h:79: bool
Initialize(base::StringPiece server_config,
On 2014/03/12 23:13:20, wtc wrote:
> 
> Nit: if the Initialize method is similar to InitializeFrom, I suggest listing
> them next to each other.

Done.

> Please document what the bool return value means. Since InitializeFrom returns
> void, I was surprised to see Initialize return bool.
> 
> Also, I am wondering why Initialize needs a |QuicWallTime now| input, but
> InitializeFrom doesn't.

Good point.  I've added a comment.  The difference between the methods is that
Initialize() needs to parse and evaluate the server config, whereas
InitializeFrom simply copies whatever exists.

https://codereview.chromium.org/196403002/diff/60001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.h:82: const std::vector<std::string>&
certs,
On 2014/03/12 23:13:20, wtc wrote:
> 
> I suggest listing the first four arguments in the same order as the
> corresponding class members: server_config, source_address_token, certs,
> signature.
> 
> If you reorder them, please be careful because three of the arguments have the
> same type, so the compiler cannot distinguish between them if you make a
> mistake.

Done.  However, the certs class member is actually 3rd, not 4th, so that made it
easier.

https://codereview.chromium.org/196403002/diff/60001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.h (right):

https://codereview.chromium.org/196403002/diff/60001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.h:108: void
SaveQuicServerInfo(QuicCryptoClientConfig::CachedState* cached);
On 2014/03/12 23:13:20, wtc wrote:
> 
> 1. Please document that we should call SaveQuicServerInfo whenever we call
> cached->SetProofValid().

Done.

> Perhaps this should be done by cached->SetProofValid()?

It could be but that would increase, rather than decrease the consistency of
this code with respect to the internal version. In any case, this will soon be
moving :>

> 2. |cached| should be a const pointer or const reference.

Good point.  Done.

https://codereview.chromium.org/196403002/diff/60001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.h:138: scoped_ptr<QuicServerInfo>
quic_server_info_;
On 2014/03/12 23:13:20, wtc wrote:
> 
> Nit: because of the "The result of certificate verification." comment, it
would
> be nice to add a blank line to make it clear this is not part of that group.

Done.

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

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-03-13 16:40:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/196403002/80001

[commit-bot: I haz the power, 2014-03-13 18:10:58]

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

[commit-bot: I haz the power, 2014-03-13 18:10:59]

Retried try job too often on ios_dbg_simulator for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[Ryan Hamilton, 2014-03-13 22:55:17]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-03-13 22:57:08]

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

[commit-bot: I haz the power, 2014-03-14 00:24:38]

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

[commit-bot: I haz the power, 2014-03-14 10:08:07]

Change committed as 257063