QUIC - don't load data from disk cache if alternate protocol map doesn't

QUIC - don't load data from disk cache if alternate protocol map doesn't
have an entry.

This change tests if we will speed up completion of crypto handshake.
We wouldn't have persisted quic server information if we haven't spoken
QUIC to the server. In such cases, loading data from disk cache is not
on the critical path.

This is controlled via a FieldTrial to see if this change would have
positive impact or not.

R=rch@chromium.org

Committed: https://crrev.com/b4af4ed1886d04cc42e6a7a6e6f9d51261caa7fc
Cr-Commit-Position: refs/heads/master@{#309116}

[ramant (doing other things), 2014-12-18 18:34:52]

Patchset #1 (id:1) has been deleted

[ramant (doing other things), 2014-12-18 18:37:03]

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

[ramant (doing other things), 2014-12-18 18:37:03]

Hi Ryan,
  Implemented what we had talked about not waiting to load from disk cache if
there is not entry in AlternateProtocolMap. If this change looks good, will
create an experiment.

thanks
raman

https://codereview.chromium.org/811073004/diff/20001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/811073004/diff/20001/chrome/browser/io_thread...
chrome/browser/io_thread.h:195: Optional<bool>
quic_load_server_info_if_already_spoke_quic;
hi Ryan,
  I am not sure about the name. Would appreciate better name.

thanks.

[Ryan Hamilton, 2014-12-18 19:19:12]

https://codereview.chromium.org/811073004/diff/20001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/811073004/diff/20001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:1377: quic_trial_params,
"load_server_info_if_already_spoke_quic"), "true");
To confirm, are you expecting to get the old behavior in the default case, or
the new behavrior in the default case?

https://codereview.chromium.org/811073004/diff/20001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/811073004/diff/20001/chrome/browser/io_thread...
chrome/browser/io_thread.h:195: Optional<bool>
quic_load_server_info_if_already_spoke_quic;
On 2014/12/18 18:37:03, ramant wrote:
> hi Ryan,
>   I am not sure about the name. Would appreciate better name.
> 
> thanks.

Hm. Good question. To be clear this controls if we should read from the disk
cache when the server is not in HSP. If it *is* in HSP, then we always read,
regardless of the value of this flag.

I'd be inclined to name it something like:

quic_load_server_info_for_unlikely_servers

or perhaps "new_servers" or some such?

[ramant (doing other things), 2014-12-18 20:51:10]

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

[ramant (doing other things), 2014-12-18 20:51:11]

Many many thanks Ryan for the name suggestion. PTAL.

-raman

https://codereview.chromium.org/811073004/diff/20001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/811073004/diff/20001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:1377: quic_trial_params,
"load_server_info_if_already_spoke_quic"), "true");
On 2014/12/18 19:19:12, Ryan Hamilton wrote:
> To confirm, are you expecting to get the old behavior in the default case, or
> the new behavrior in the default case?

Old behavior in the default case. New behavior when the FieldTrial is on.

https://codereview.chromium.org/811073004/diff/20001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/811073004/diff/20001/chrome/browser/io_thread...
chrome/browser/io_thread.h:195: Optional<bool>
quic_load_server_info_if_already_spoke_quic;
On 2014/12/18 19:19:12, Ryan Hamilton wrote:
> On 2014/12/18 18:37:03, ramant wrote:
> > hi Ryan,
> >   I am not sure about the name. Would appreciate better name.
> > 
> > thanks.
> 
> Hm. Good question. To be clear this controls if we should read from the disk
> cache when the server is not in HSP. If it *is* in HSP, then we always read,
> regardless of the value of this flag.
> 
> I'd be inclined to name it something like:
> 
> quic_load_server_info_for_unlikely_servers
> 
> or perhaps "new_servers" or some such?

Thanks Ryan for the above suggestion. Renamed the variable.

Done.

[Ryan Hamilton, 2014-12-18 21:12:55]

lgtm

Thanks for doing this!

https://codereview.chromium.org/811073004/diff/40001/chrome/browser/io_thread...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/811073004/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread_unittest.cc:305:
EXPECT_TRUE(params.quic_disable_loading_server_info_for_new_servers);
please add an EXPECT_FALSE for this field up to the first test (Which verifies
the defaults).

[ramant (doing other things), 2014-12-18 23:05:55]

Thanks very much Ryan,
raman

https://codereview.chromium.org/811073004/diff/40001/chrome/browser/io_thread...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/811073004/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread_unittest.cc:305:
EXPECT_TRUE(params.quic_disable_loading_server_info_for_new_servers);
On 2014/12/18 21:12:55, Ryan Hamilton wrote:
> please add an EXPECT_FALSE for this field up to the first test (Which verifies
> the defaults).

Done.

[ramant (doing other things), 2014-12-18 23:05:59]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-12-18 23:06:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/811073004/60001

[commit-bot: I haz the power, 2014-12-19 00:36:32]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2014-12-19 00:37:27]

Patchset 3 (id:??) landed as
https://crrev.com/b4af4ed1886d04cc42e6a7a6e6f9d51261caa7fc
Cr-Commit-Position: refs/heads/master@{#309116}