QUIC - Allow cronet apps to specify how many server configs are to be

QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

Added "max_server_configs_stored_in_properties" cronet
experiment options.

Will delete support for "store_server_configs_in_properties",
 after all cronet apps stop using it. Added backward
compatibility code that sets
number_of_server_configs_stored_in_properties to 5
when this option is enabled.

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

Committed: https://crrev.com/6971c17d82c972e7bd26eb2e463e89b1f4922047
Cr-Commit-Position: refs/heads/master@{#369825}

[ramant (doing other things), 2016-01-09 03:34:45]

Description was changed from

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

TODO: maintain the MRU order of server configs when they are persisted
to disk (use list instead of dictionary).
==========

to

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).
==========

[ramant (doing other things), 2016-01-09 03:39:06]

Patchset #1 (id:1) has been deleted

[ramant (doing other things), 2016-01-09 03:39:15]

The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-09 03:39:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1572753003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1572753003/20001

[ramant (doing other things), 2016-01-09 03:39:52]

Description was changed from

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).
==========

to

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

Added "number_of_server_configs_stored_in_properties" cronet
experiment options.
==========

[ramant (doing other things), 2016-01-09 03:47:23]

Description was changed from

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

Added "number_of_server_configs_stored_in_properties" cronet
experiment options.
==========

to

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

Added "number_of_server_configs_stored_in_properties" cronet
experiment options.

Will delete support for "store_server_configs_in_properties", after all cronet
apps stop using it. Added backward comparability code that sets
number_of_server_configs_stored_in_properties to 5 when this option is enabled.
==========

[ramant (doing other things), 2016-01-09 03:47:52]

Description was changed from

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

Added "number_of_server_configs_stored_in_properties" cronet
experiment options.

Will delete support for "store_server_configs_in_properties", after all cronet
apps stop using it. Added backward comparability code that sets
number_of_server_configs_stored_in_properties to 5 when this option is enabled.
==========

to

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

Added "number_of_server_configs_stored_in_properties" cronet
experiment options.

Will delete support for "store_server_configs_in_properties",
 after all cronet apps stop using it. Added backward 
comparability code that sets number_of_server_configs_stored_in_properties to 5
when this option is enabled.
==========

[ramant (doing other things), 2016-01-09 03:48:18]

Description was changed from

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

Added "number_of_server_configs_stored_in_properties" cronet
experiment options.

Will delete support for "store_server_configs_in_properties",
 after all cronet apps stop using it. Added backward 
comparability code that sets number_of_server_configs_stored_in_properties to 5
when this option is enabled.
==========

to

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

Added "number_of_server_configs_stored_in_properties" cronet
experiment options.

Will delete support for "store_server_configs_in_properties",
 after all cronet apps stop using it. Added backward 
comparability code that sets 
number_of_server_configs_stored_in_properties to 5
when this option is enabled.
==========

[commit-bot: I haz the power, 2016-01-09 04:49:25]

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

[commit-bot: I haz the power, 2016-01-09 04:49:25]

Dry run: This issue passed the CQ dry run.

[ramant (doing other things), 2016-01-11 18:37:41]

Patchset #1 (id:20001) has been deleted

[ramant (doing other things), 2016-01-11 18:38:32]

Description was changed from

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

Added "number_of_server_configs_stored_in_properties" cronet
experiment options.

Will delete support for "store_server_configs_in_properties",
 after all cronet apps stop using it. Added backward 
comparability code that sets 
number_of_server_configs_stored_in_properties to 5
when this option is enabled.
==========

to

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

Added "number_of_server_configs_stored_in_properties" cronet
experiment options.

Will delete support for "store_server_configs_in_properties",
 after all cronet apps stop using it. Added backward 
comparability code that sets 
number_of_server_configs_stored_in_properties to 5
when this option is enabled.

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

[ramant (doing other things), 2016-01-11 18:38:32]

rtenneti@chromium.org changed reviewers:
+ mef@chromium.org, rch@chromium.org

[ramant (doing other things), 2016-01-11 18:38:45]

The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-11 18:39:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1572753003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1572753003/40001

[ramant (doing other things), 2016-01-11 18:47:09]

Hi Ryan and Misha,
  One of the App which uses cronet has more than 5 QUIC servers. This CL gives
them the flexibility to specify the number of QUIC servers they would like to
persist. When we first implemented it, we had youtube in mind and we didn't want
app startup to take too much time, thus we decided to persist only 5 QUIC
servers.

  Each server config takes 5k bytes, cronet app's could persist lot more than 5
QUIC server configs without having an impact on app's startup performance. Would
like to leave it the App developers to decide the right number for their app.

  Would like to retire the old option after all cronet App's have switched over
to the new option.

thanks,
raman

[commit-bot: I haz the power, 2016-01-11 20:08:54]

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

[commit-bot: I haz the power, 2016-01-11 20:08:55]

Dry run: This issue passed the CQ dry run.

[Ryan Hamilton, 2016-01-12 19:54:56]

https://codereview.chromium.org/1572753003/diff/60001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/1572753003/diff/60001/net/http/http_network_s...
net/http/http_network_session.h:158: int
quic_number_of_server_configs_stored_in_properties;
nit: s/number_of/max/

https://codereview.chromium.org/1572753003/diff/60001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1572753003/diff/60001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:661:
number_of_server_configs_stored_in_properties_);
I'm a bit surprised to see this code living in QuicStreamFactory. Is there a way
to do this logic directly in HttpServerProperties<something>

[ramant (doing other things), 2016-01-13 22:54:28]

Description was changed from

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

Added "number_of_server_configs_stored_in_properties" cronet
experiment options.

Will delete support for "store_server_configs_in_properties",
 after all cronet apps stop using it. Added backward 
comparability code that sets 
number_of_server_configs_stored_in_properties to 5
when this option is enabled.

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

to

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

Added "max_server_configs_stored_in_properties" cronet
experiment options.

Will delete support for "store_server_configs_in_properties",
 after all cronet apps stop using it. Added backward 
comparability code that sets 
number_of_server_configs_stored_in_properties to 5
when this option is enabled.

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

[ramant (doing other things), 2016-01-14 00:14:01]

Thanks Ryan. Made the changes. PTAL.

https://codereview.chromium.org/1572753003/diff/60001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/1572753003/diff/60001/net/http/http_network_s...
net/http/http_network_session.h:158: int
quic_number_of_server_configs_stored_in_properties;
On 2016/01/12 19:54:56, Ryan Hamilton wrote:
> nit: s/number_of/max/

Done.

https://codereview.chromium.org/1572753003/diff/60001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1572753003/diff/60001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:661:
number_of_server_configs_stored_in_properties_);
On 2016/01/12 19:54:56, Ryan Hamilton wrote:
> I'm a bit surprised to see this code living in QuicStreamFactory. Is there a
way
> to do this logic directly in HttpServerProperties<something>

Done.

[ramant (doing other things), 2016-01-14 00:14:07]

The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-14 00:15:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1572753003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1572753003/80001

[commit-bot: I haz the power, 2016-01-14 02:23:02]

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

[commit-bot: I haz the power, 2016-01-14 02:23:02]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
  linux_android_rel_ng on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[ramant (doing other things), 2016-01-14 20:30:51]

The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-14 20:34:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1572753003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1572753003/100001

[ramant (doing other things), 2016-01-14 20:47:28]

Hi Ryan,
  Merged with TOT and thus it pulled in lot of changes (android builds were
failing, thus rebase'ed with TOT).

thanks
raman

https://codereview.chromium.org/1572753003/diff/100001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1572753003/diff/100001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:675: if (max_server_configs_stored_in_properties
> 0) {
When QuicStreamFactory was created in HttpNetworkSession, HttpServerProperties's
data was not updated. Thus passed it as argument.

https://codereview.chromium.org/1572753003/diff/100001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1572753003/diff/100001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:279: return quic_server_info_factory_.get() !=
nullptr;
Hi Ryan,
  This is a bug. I had noticed while testing. Added the test for this method in
quic_stream_factory_test.cc.

https://codereview.chromium.org/1572753003/diff/100001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/1572753003/diff/100001/net/quic/quic_stream_f...
net/quic/quic_stream_factory_test.cc:245:
EXPECT_FALSE(factory_->has_quic_server_info_factory());
Added test for has_quic_server_info_factory().

[commit-bot: I haz the power, 2016-01-14 21:44:24]

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

[commit-bot: I haz the power, 2016-01-14 21:44:25]

Dry run: This issue passed the CQ dry run.

[Ryan Hamilton, 2016-01-15 00:56:03]

lgtm

[ramant (doing other things), 2016-01-15 01:14:00]

On 2016/01/15 00:56:03, Ryan Hamilton wrote:
> lgtm

Thanks much Ryan.

Hi Misha,
  Would appreciate if you could take a look at this change.

thanks
raman

[mef, 2016-01-15 16:12:32]

Sorry for the delay.

https://codereview.chromium.org/1572753003/diff/100001/components/cronet/url_...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/1572753003/diff/100001/components/cronet/url_...
components/cronet/url_request_context_config.cc:82: // Added this for backward
compatability.
nit: compatability -> compatibility

https://codereview.chromium.org/1572753003/diff/100001/components/cronet/url_...
components/cronet/url_request_context_config.cc:87:
net::kMaxQuicServersToPersist);
#include "net/http/http_server_properties.h"

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_network_...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_network_...
net/http/http_network_session.cc:117:
quic_max_server_configs_stored_in_properties(0),
BUG? 

See mru_cache.h:

  enum { NO_AUTO_EVICT = 0 };

If we default to 0, would it mean that we'll store unlimited number of server
configs? Is that what we expect?

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_network_...
net/http/http_network_session.cc:242:
http_server_properties_->set_max_server_configs_stored_in_properties(
There was no params.quic_store_server_configs_in_properties here before?

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_server_p...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_server_p...
net/http/http_server_properties.h:219: const size_t kMaxQuicServersToPersist =
5;
should this remain int?

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_server_p...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_server_p...
net/http/http_server_properties_impl.cc:684: void
HttpServerPropertiesImpl::set_max_server_configs_stored_in_properties(
Should this be SetMaxServerConfigsStoredInProperties given that it is not just a
setter, but has a considerable side effect?

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_server_p...
net/http/http_server_properties_impl.cc:692: for
(QuicServerInfoMap::reverse_iterator it = quic_server_info_map_.rbegin();
Microoptimization: quic_server_info_map_.ShrinkToSize first, and than copy
everything into new map. This will avoid copy-and-delete of entries that don't
fit into new map.

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_server_p...
File net/http/http_server_properties_impl.h (right):

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_server_p...
net/http/http_server_properties_impl.h:193: int
max_server_configs_stored_in_properties_;
do we want int or do we want size_t?

[ramant (doing other things), 2016-01-15 18:57:01]

Thanks Misha. PTAL.

https://codereview.chromium.org/1572753003/diff/100001/components/cronet/url_...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/1572753003/diff/100001/components/cronet/url_...
components/cronet/url_request_context_config.cc:82: // Added this for backward
compatability.
On 2016/01/15 16:12:32, mef wrote:
> nit: compatability -> compatibility

Done.

https://codereview.chromium.org/1572753003/diff/100001/components/cronet/url_...
components/cronet/url_request_context_config.cc:87:
net::kMaxQuicServersToPersist);
On 2016/01/15 16:12:32, mef wrote:
> #include "net/http/http_server_properties.h"

Done.

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_network_...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_network_...
net/http/http_network_session.cc:117:
quic_max_server_configs_stored_in_properties(0),
On 2016/01/15 16:12:32, mef wrote:
> BUG? 
> 
> See mru_cache.h:
> 
>   enum { NO_AUTO_EVICT = 0 };
> 
> If we default to 0, would it mean that we'll store unlimited number of server
> configs? Is that what we expect?

We create PropertiesBasedQuicServerInfoFactory only when 
quic_max_server_configs_stored_in_properties is not 0 in 
QuicStreamFactory::QuicStreamFactory.

We don't enable this feature unless we create
PropertiesBasedQuicServerInfoFactory object, otherwise we will use disk cache
based cache.


https://codereview.chromium.org/1572753003/diff/100001/net/quic/quic_stream_f...

We add entries to QuicServerInfoMap in the following code only when 
PropertiesBasedQuicServerInfo is created otherwise we choose
DiskCacheBasedQuicServerInfo.

https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/crypto/pr...

https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/crypto/pr...

If quic_max_server_configs_stored_in_properties > 0, then only we add entries
into the map, otherwise that map is not used.

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_network_...
net/http/http_network_session.cc:242:
http_server_properties_->set_max_server_configs_stored_in_properties(
On 2016/01/15 16:12:32, mef wrote:
> There was no params.quic_store_server_configs_in_properties here before?

This flag used to be saved in QuicStreamFactory.h and HttpServerProperties had a
hard coded size.

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_server_p...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_server_p...
net/http/http_server_properties.h:219: const size_t kMaxQuicServersToPersist =
5;
On 2016/01/15 16:12:32, mef wrote:
> should this remain int?

Used size_t for the size everywhere to be consistent with MRUCache's size
argument.

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_server_p...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_server_p...
net/http/http_server_properties_impl.cc:684: void
HttpServerPropertiesImpl::set_max_server_configs_stored_in_properties(
On 2016/01/15 16:12:32, mef wrote:
> Should this be SetMaxServerConfigsStoredInProperties given that it is not just
a
> setter, but has a considerable side effect?

Good point.

Done.

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_server_p...
net/http/http_server_properties_impl.cc:692: for
(QuicServerInfoMap::reverse_iterator it = quic_server_info_map_.rbegin();
On 2016/01/15 16:12:32, mef wrote:
> Microoptimization: quic_server_info_map_.ShrinkToSize first, and than copy
> everything into new map. This will avoid copy-and-delete of entries that don't
> fit into new map.

+1. Thanks for the above optimization.

Done.

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_server_p...
File net/http/http_server_properties_impl.h (right):

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_server_p...
net/http/http_server_properties_impl.h:193: int
max_server_configs_stored_in_properties_;
On 2016/01/15 16:12:32, mef wrote:
> do we want int or do we want size_t?

Used size_t everywhere.

[ramant (doing other things), 2016-01-15 18:57:50]

The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-15 18:59:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1572753003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1572753003/120001

[mef, 2016-01-15 20:01:36]

lgtm

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_network_...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/1572753003/diff/100001/net/http/http_network_...
net/http/http_network_session.cc:117:
quic_max_server_configs_stored_in_properties(0),
On 2016/01/15 18:57:00, ramant wrote:
> On 2016/01/15 16:12:32, mef wrote:
> > BUG? 
> > 
> > See mru_cache.h:
> > 
> >   enum { NO_AUTO_EVICT = 0 };
> > 
> > If we default to 0, would it mean that we'll store unlimited number of
server
> > configs? Is that what we expect?
> 
> We create PropertiesBasedQuicServerInfoFactory only when 
> quic_max_server_configs_stored_in_properties is not 0 in 
> QuicStreamFactory::QuicStreamFactory.
> 
> We don't enable this feature unless we create
> PropertiesBasedQuicServerInfoFactory object, otherwise we will use disk cache
> based cache.
> 
> 
>
https://codereview.chromium.org/1572753003/diff/100001/net/quic/quic_stream_f...
> 
> We add entries to QuicServerInfoMap in the following code only when 
> PropertiesBasedQuicServerInfo is created otherwise we choose
> DiskCacheBasedQuicServerInfo.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/crypto/pr...
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/crypto/pr...
> 
> If quic_max_server_configs_stored_in_properties > 0, then only we add entries
> into the map, otherwise that map is not used.

Thanks for the explanation! I think it is a bit confusing, but not broken.

[ramant (doing other things), 2016-01-15 20:03:50]

On 2016/01/15 20:01:36, mef wrote:
> lgtm
> 
>
https://codereview.chromium.org/1572753003/diff/100001/net/http/http_network_...
> File net/http/http_network_session.cc (right):
> 
>
https://codereview.chromium.org/1572753003/diff/100001/net/http/http_network_...
> net/http/http_network_session.cc:117:
> quic_max_server_configs_stored_in_properties(0),
> On 2016/01/15 18:57:00, ramant wrote:
> > On 2016/01/15 16:12:32, mef wrote:
> > > BUG? 
> > > 
> > > See mru_cache.h:
> > > 
> > >   enum { NO_AUTO_EVICT = 0 };
> > > 
> > > If we default to 0, would it mean that we'll store unlimited number of
> server
> > > configs? Is that what we expect?
> > 
> > We create PropertiesBasedQuicServerInfoFactory only when 
> > quic_max_server_configs_stored_in_properties is not 0 in 
> > QuicStreamFactory::QuicStreamFactory.
> > 
> > We don't enable this feature unless we create
> > PropertiesBasedQuicServerInfoFactory object, otherwise we will use disk
cache
> > based cache.
> > 
> > 
> >
>
https://codereview.chromium.org/1572753003/diff/100001/net/quic/quic_stream_f...
> > 
> > We add entries to QuicServerInfoMap in the following code only when 
> > PropertiesBasedQuicServerInfo is created otherwise we choose
> > DiskCacheBasedQuicServerInfo.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/crypto/pr...
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/crypto/pr...
> > 
> > If quic_max_server_configs_stored_in_properties > 0, then only we add
entries
> > into the map, otherwise that map is not used.
> 
> Thanks for the explanation! I think it is a bit confusing, but not broken.

Thanks very much Misha,
raman

[ramant (doing other things), 2016-01-15 20:04:53]

Description was changed from

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

Added "max_server_configs_stored_in_properties" cronet
experiment options.

Will delete support for "store_server_configs_in_properties",
 after all cronet apps stop using it. Added backward 
comparability code that sets 
number_of_server_configs_stored_in_properties to 5
when this option is enabled.

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

to

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

Added "max_server_configs_stored_in_properties" cronet
experiment options.

Will delete support for "store_server_configs_in_properties",
 after all cronet apps stop using it. Added backward 
compatibility code that sets 
number_of_server_configs_stored_in_properties to 5
when this option is enabled.

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

[ramant (doing other things), 2016-01-15 20:05:38]

The CQ bit was unchecked by rtenneti@chromium.org

[ramant (doing other things), 2016-01-15 20:05:43]

The CQ bit was checked by rtenneti@chromium.org

[ramant (doing other things), 2016-01-15 20:05:43]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1572753003/#ps120001
(title: "Fix comments in Patch set 4")

[commit-bot: I haz the power, 2016-01-15 20:06:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1572753003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1572753003/120001

[commit-bot: I haz the power, 2016-01-15 20:12:17]

Description was changed from

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

Added "max_server_configs_stored_in_properties" cronet
experiment options.

Will delete support for "store_server_configs_in_properties",
 after all cronet apps stop using it. Added backward 
compatibility code that sets 
number_of_server_configs_stored_in_properties to 5
when this option is enabled.

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

to

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

Added "max_server_configs_stored_in_properties" cronet
experiment options.

Will delete support for "store_server_configs_in_properties",
 after all cronet apps stop using it. Added backward 
compatibility code that sets 
number_of_server_configs_stored_in_properties to 5
when this option is enabled.

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

[commit-bot: I haz the power, 2016-01-15 20:12:18]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-01-15 20:13:17]

Description was changed from

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

Added "max_server_configs_stored_in_properties" cronet
experiment options.

Will delete support for "store_server_configs_in_properties",
 after all cronet apps stop using it. Added backward 
compatibility code that sets 
number_of_server_configs_stored_in_properties to 5
when this option is enabled.

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

to

==========
QUIC - Allow cronet apps to specify how many server configs are to be
persisted in prefs file (local_prefs.json).

Added "max_server_configs_stored_in_properties" cronet
experiment options.

Will delete support for "store_server_configs_in_properties",
 after all cronet apps stop using it. Added backward
compatibility code that sets
number_of_server_configs_stored_in_properties to 5
when this option is enabled.

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

Committed: https://crrev.com/6971c17d82c972e7bd26eb2e463e89b1f4922047
Cr-Commit-Position: refs/heads/master@{#369825}
==========

[commit-bot: I haz the power, 2016-01-15 20:13:18]

Patchset 5 (id:??) landed as
https://crrev.com/6971c17d82c972e7bd26eb2e463e89b1f4922047
Cr-Commit-Position: refs/heads/master@{#369825}