[Chromecast] Use base::FeatureList to control features.

[Chromecast] Use base::FeatureList to control features.

In Chromium, Finch-enabled features are controlled through base::FeatureList,
a class which abstracts the experiment framework and developer overrides
from client code. Though Chromecast's experiment framework is fundamentally
different (in that it is server-driven) Cast builds can still make use of
this class. Introduce some utilities to help.

At boot-up, the pref store will be queried for experiment configs, which
were cached to disk on the most recent config fetch from the last boot
cycle. If a developer overrides these features from the command line,
that value takes precedence. These features will be used to initialize
base::FeatureList, which can then be statically queried from any client
code that depends on //base.

This patch does not actually introduce or convert any existing features
to use this framework.

BUG=714291
BUG= internal b/35424335

Review-Url: https://codereview.chromium.org/2825873002
Cr-Commit-Position: refs/heads/master@{#466507}
Committed: https://chromium.googlesource.com/chromium/src/+/ef03d2b9a67352094516f4e866373378fb961ee8

[slan, 2017-04-18 21:34:11]

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

[commit-bot: I haz the power, 2017-04-18 21:34:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 22:48:49]

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

[commit-bot: I haz the power, 2017-04-18 22:48:50]

Dry run: This issue passed the CQ dry run.

[slan, 2017-04-19 22:57:12]

slan@chromium.org changed reviewers:
+ halliwell@chromium.org, maclellant@chromium.org

[slan, 2017-04-19 22:57:12]

Internal change here:

https://eureka-internal-review.git.corp.google.com/#/c/71738/

I'd like to add an end-to-end test before this lands... still figuring out how
to do that internally.

[halliwell, 2017-04-20 02:49:31]

A few nits but looking nice!

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
File chromecast/base/cast_features.cc (right):

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:25:
base::LazyInstance<std::unordered_set<int32_t>>::DestructorAtExit
don't we usually just do leaky unless there's a strong reason not to?

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:61: base::DictionaryValue::Iterator
iterator(dcs_features);
nit, can init iterator inside for (...)?

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:109: base::DictionaryValue::Iterator
params_iterator(*params_dict);
ditto init in for(...)

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:142: base::DictionaryValue::Iterator
iterator(features);
ditto

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:159: const auto& param_key =
params_iterator.key();
ditto

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
File chromecast/base/cast_features.h (right):

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
chromecast/base/cast_features.h:23: // PrefService in the browser. Thread-safe.
I'm not sure "Thread-safe" completely characterises these functions.  It's more
just that you have to be certain this one that gets called once is finished,
then you can call GetDCSExperimentIds on any threads.

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
chromecast/base/cast_features.h:26: const std::string& cmd_line_enable_features,
nit, include string

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
chromecast/base/cast_features.h:37: std::unordered_set<int32_t>
GetDCSExperimentIds();
include <cstdint>

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
chromecast/base/cast_features.h:37: std::unordered_set<int32_t>
GetDCSExperimentIds();
how big is this set?  Are users expecting to modify it?  If not, should return
const ref.

[slan, 2017-04-20 16:26:02]

Thanks for the review. Comments addressed.

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
File chromecast/base/cast_features.cc (right):

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:25:
base::LazyInstance<std::unordered_set<int32_t>>::DestructorAtExit
On 2017/04/20 02:49:30, halliwell wrote:
> don't we usually just do leaky unless there's a strong reason not to?

Chromium seems to be fairly split. There are 17 uses of DestructorAtExit to wrap
static std containers: 

https://cs.chromium.org/search/?q=base::LazyInstance<std::(.*)>::DestructorAt...

... and 28 uses of Leaky:

https://cs.chromium.org/search/?q=base::LazyInstance<std::(.*)>::Leaky


Switched to Leaky, as it doesn't really matter if the dtor is called here.

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:61: base::DictionaryValue::Iterator
iterator(dcs_features);
On 2017/04/20 02:49:30, halliwell wrote:
> nit, can init iterator inside for (...)?

Done, also updated the type and variable name to be more brief.

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:109: base::DictionaryValue::Iterator
params_iterator(*params_dict);
On 2017/04/20 02:49:30, halliwell wrote:
> ditto init in for(...)

Done.

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:142: base::DictionaryValue::Iterator
iterator(features);
On 2017/04/20 02:49:30, halliwell wrote:
> ditto

Done.

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:159: const auto& param_key =
params_iterator.key();
On 2017/04/20 02:49:31, halliwell wrote:
> ditto

Done.

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
File chromecast/base/cast_features.h (right):

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
chromecast/base/cast_features.h:23: // PrefService in the browser. Thread-safe.
On 2017/04/20 02:49:31, halliwell wrote:
> I'm not sure "Thread-safe" completely characterises these functions.  It's
more
> just that you have to be certain this one that gets called once is finished,
> then you can call GetDCSExperimentIds on any threads.

You're right. Done.

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
chromecast/base/cast_features.h:26: const std::string& cmd_line_enable_features,
On 2017/04/20 02:49:31, halliwell wrote:
> nit, include string

Done.

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
chromecast/base/cast_features.h:37: std::unordered_set<int32_t>
GetDCSExperimentIds();
On 2017/04/20 02:49:31, halliwell wrote:
> include <cstdint>

Done.

https://codereview.chromium.org/2825873002/diff/20001/chromecast/base/cast_fe...
chromecast/base/cast_features.h:37: std::unordered_set<int32_t>
GetDCSExperimentIds();
On 2017/04/20 02:49:31, halliwell wrote:
> how big is this set?  Are users expecting to modify it?  If not, should return
> const ref.

This set is read-only. It will only ever be as large as the number of concurrent
experiments running on a single device. In practice, this number will be < 5. 

I remember reading somewhere that a copy should be returned for small container,
but I can't find the documentation to back that up, so... Done.

[maclellant, 2017-04-20 17:44:22]

LGTM % comments

https://codereview.chromium.org/2825873002/diff/40001/chromecast/base/cast_fe...
File chromecast/base/cast_features.cc (right):

https://codereview.chromium.org/2825873002/diff/40001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:88: feature_list->RegisterFieldTrialOverride(
Just checking, but it's safe for field trial override, even if the feature
doesn't exist/isn't registered? Since overrides are saved to pref file, they
will persist through platform upgrade that may remove a given feature.

https://codereview.chromium.org/2825873002/diff/40001/chromecast/base/cast_fe...
File chromecast/base/cast_features_unittest.cc (right):

https://codereview.chromium.org/2825873002/diff/40001/chromecast/base/cast_fe...
chromecast/base/cast_features_unittest.cc:98: ASSERT_EQ(3.14159,
base::GetFieldTrialParamByFeatureAsDouble(
Should these assertions be using ASSERT_NEAR instead of ASSERT_EQ?
I guess double->string->double on the same machine will yield same result, but
only if conversion function doesn't shave precision for speed.

[halliwell, 2017-04-20 19:58:10]

lgtm % Tavis's questions.

https://codereview.chromium.org/2825873002/diff/40001/chromecast/base/cast_fe...
File chromecast/base/cast_features_unittest.cc (right):

https://codereview.chromium.org/2825873002/diff/40001/chromecast/base/cast_fe...
chromecast/base/cast_features_unittest.cc:98: ASSERT_EQ(3.14159,
base::GetFieldTrialParamByFeatureAsDouble(
On 2017/04/20 17:44:21, maclellant wrote:
> Should these assertions be using ASSERT_NEAR instead of ASSERT_EQ?
> I guess double->string->double on the same machine will yield same result, but
> only if conversion function doesn't shave precision for speed.

It is possible to double->string->double with round-trip guarantee.  I'd
actually like to know whether our goal is to support this (and if so,
potentially to add some more tests to ensure it).  I couldn't see at a quick
glance what the conversion functions do that you chose to use.

If we don't support this, should there be some warning around this for users of
doubles?

[slan, 2017-04-20 20:51:55]

PTAL

https://codereview.chromium.org/2825873002/diff/40001/chromecast/base/cast_fe...
File chromecast/base/cast_features.cc (right):

https://codereview.chromium.org/2825873002/diff/40001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:88: feature_list->RegisterFieldTrialOverride(
On 2017/04/20 17:44:21, maclellant wrote:
> Just checking, but it's safe for field trial override, even if the feature
> doesn't exist/isn't registered? Since overrides are saved to pref file, they
> will persist through platform upgrade that may remove a given feature.

Right, it works both ways: 
1) If a feature is removed in the most recent release, a stale feature can be
registered here without issue. 
2) If a new feature is added, it can be queried safely from client code.

https://codereview.chromium.org/2825873002/diff/40001/chromecast/base/cast_fe...
File chromecast/base/cast_features_unittest.cc (right):

https://codereview.chromium.org/2825873002/diff/40001/chromecast/base/cast_fe...
chromecast/base/cast_features_unittest.cc:98: ASSERT_EQ(3.14159,
base::GetFieldTrialParamByFeatureAsDouble(
On 2017/04/20 19:58:10, halliwell wrote:
> On 2017/04/20 17:44:21, maclellant wrote:
> > Should these assertions be using ASSERT_NEAR instead of ASSERT_EQ?
> > I guess double->string->double on the same machine will yield same result,
but
> > only if conversion function doesn't shave precision for speed.
> 
> It is possible to double->string->double with round-trip guarantee.  I'd
> actually like to know whether our goal is to support this (and if so,
> potentially to add some more tests to ensure it).  I couldn't see at a quick
> glance what the conversion functions do that you chose to use.
> 
> If we don't support this, should there be some warning around this for users
of
> doubles?

Fair questions.

The Feature framework uses base::StringToDouble() to deserialize the dictionary
cached to disk:
https://cs.chromium.org/chromium/src/base/metrics/field_trial_params.cc?rcl=9...

And I use base::DoubleToString() to serialize when writing to disk:
https://codereview.chromium.org/2825873002/diff/40001/chromecast/base/cast_fe...

My assumption is that these functions are more or less guaranteed to be
symmetric, but the unittests do not actually explicitly check this:
https://cs.chromium.org/chromium/src/base/strings/string_number_conversions_u...

I'd be fine putting a caveat in the docs that you shouldn't rely on perfect
precision for double params. I think in practice, the guarantee offered by
Chromium utils will be more than enough.

[slan, 2017-04-20 20:52:45]

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

[commit-bot: I haz the power, 2017-04-20 20:53:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maclellant, 2017-04-20 21:00:04]

SGTM. As long as our features don't depend on exact bit-level matching for
float/double parameters, this really shouldn't be an issue.

[commit-bot: I haz the power, 2017-04-20 21:04:21]

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

[commit-bot: I haz the power, 2017-04-20 21:04:22]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[slan, 2017-04-21 19:15:56]

slan@chromium.org changed reviewers:
+ asvitkine@chromium.org

[slan, 2017-04-21 19:15:57]

+Alexei

Alexei, would you mind taking a quick pass on cast_features.cc to inspect the
initialization of FeatureList? Thanks!

[Alexei Svitkine (slow), 2017-04-21 19:48:02]

generally, lg - some comments below

https://codereview.chromium.org/2825873002/diff/80001/chromecast/base/cast_fe...
File chromecast/base/cast_features.cc (right):

https://codereview.chromium.org/2825873002/diff/80001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:31: int32_t id;
Nit: Move this into the loop right before it's used.

https://codereview.chromium.org/2825873002/diff/80001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:32: auto ids =
base::MakeUnique<std::unordered_set<int32_t>>();
I don't think you need to allocate this on the heap.

"std::unordered_set<int32_t> ids;" should do. The swap call on line 40 swaps the
internal contents of the data structure, not the object itself.

https://codereview.chromium.org/2825873002/diff/80001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:69: //     be unique among features.
Can this sentence be expanded to mention what's being used for it? Since here
you're using it.key() which I think is the feature name?

https://codereview.chromium.org/2825873002/diff/80001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:79: const std::string& field_trial_name =
it.key();
Nit: I'd actually name this feature_name and also use it below on line 89 and
later.

This way, it's clear what it.key() is.

https://codereview.chromium.org/2825873002/diff/80001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:135: std::unique_ptr<base::DictionaryValue>
GetOverriddenFeaturesForStorage(
Nit: base::DictionaryValue is already copiable, so you can return it by value
rather than by ptr.

https://codereview.chromium.org/2825873002/diff/80001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:189: void ResetCastFeaturesForTest() {
Nit: I think convention is ForTesting rather than ForTest

https://codereview.chromium.org/2825873002/diff/80001/chromecast/base/cast_fe...
File chromecast/base/cast_features.h (right):

https://codereview.chromium.org/2825873002/diff/80001/chromecast/base/cast_fe...
chromecast/base/cast_features.h:23: // Initialize the global
base::FeatureListInstance, taking into account
Nit: base::FeatureListInstance isn't a thing - you can say "base::FeatureList
instance".

https://codereview.chromium.org/2825873002/diff/80001/chromecast/browser/test...
File chromecast/browser/test/cast_features_browsertest.cc (right):

https://codereview.chromium.org/2825873002/diff/80001/chromecast/browser/test...
chromecast/browser/test/cast_features_browsertest.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2017 :)

https://codereview.chromium.org/2825873002/diff/80001/chromecast/browser/test...
chromecast/browser/test/cast_features_browsertest.cc:8: #include
"chromecast/base/cast_features.h"
Nit: Should be the first include.

[slan, 2017-04-21 21:43:28]

Alexei, thanks for the quick review! I took most of your suggestions.

https://codereview.chromium.org/2825873002/diff/80001/chromecast/base/cast_fe...
File chromecast/base/cast_features.cc (right):

https://codereview.chromium.org/2825873002/diff/80001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:31: int32_t id;
On 2017/04/21 19:48:01, Alexei Svitkine (slow) wrote:
> Nit: Move this into the loop right before it's used.

Done.

https://codereview.chromium.org/2825873002/diff/80001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:32: auto ids =
base::MakeUnique<std::unordered_set<int32_t>>();
On 2017/04/21 19:48:01, Alexei Svitkine (slow) wrote:
> I don't think you need to allocate this on the heap.
> 
> "std::unordered_set<int32_t> ids;" should do. The swap call on line 40 swaps
the
> internal contents of the data structure, not the object itself.

Right you are, this was a relic of an older patch. Now on the stack.

https://codereview.chromium.org/2825873002/diff/80001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:69: //     be unique among features.
On 2017/04/21 19:48:01, Alexei Svitkine (slow) wrote:
> Can this sentence be expanded to mention what's being used for it? Since here
> you're using it.key() which I think is the feature name?

Sure, let me know if it's clearer now.

https://codereview.chromium.org/2825873002/diff/80001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:79: const std::string& field_trial_name =
it.key();
On 2017/04/21 19:48:01, Alexei Svitkine (slow) wrote:
> Nit: I'd actually name this feature_name and also use it below on line 89 and
> later.
> 
> This way, it's clear what it.key() is.

I like it. Done.

https://codereview.chromium.org/2825873002/diff/80001/chromecast/base/cast_fe...
chromecast/base/cast_features.cc:189: void ResetCastFeaturesForTest() {
On 2017/04/21 19:48:01, Alexei Svitkine (slow) wrote:
> Nit: I think convention is ForTesting rather than ForTest

Done.

https://codereview.chromium.org/2825873002/diff/80001/chromecast/base/cast_fe...
File chromecast/base/cast_features.h (right):

https://codereview.chromium.org/2825873002/diff/80001/chromecast/base/cast_fe...
chromecast/base/cast_features.h:23: // Initialize the global
base::FeatureListInstance, taking into account
On 2017/04/21 19:48:01, Alexei Svitkine (slow) wrote:
> Nit: base::FeatureListInstance isn't a thing - you can say "base::FeatureList
> instance".

Typo. Done.

https://codereview.chromium.org/2825873002/diff/80001/chromecast/browser/test...
File chromecast/browser/test/cast_features_browsertest.cc (right):

https://codereview.chromium.org/2825873002/diff/80001/chromecast/browser/test...
chromecast/browser/test/cast_features_browsertest.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2017/04/21 19:48:01, Alexei Svitkine (slow) wrote:
> 2017 :)

Ugh... sloppy. Done.

https://codereview.chromium.org/2825873002/diff/80001/chromecast/browser/test...
chromecast/browser/test/cast_features_browsertest.cc:8: #include
"chromecast/base/cast_features.h"
On 2017/04/21 19:48:01, Alexei Svitkine (slow) wrote:
> Nit: Should be the first include.

Done.

[Alexei Svitkine (slow), 2017-04-21 21:46:03]

LGTM

You should probably have a crbug and BUG= set for a change of this size. :)

https://codereview.chromium.org/2825873002/diff/100001/chromecast/base/cast_f...
File chromecast/base/cast_features.cc (right):

https://codereview.chromium.org/2825873002/diff/100001/chromecast/base/cast_f...
chromecast/base/cast_features.cc:71: //     Feature's name as the FieldTrial
name to ensure uniquenes.
Nit: uniqueness

[slan, 2017-04-21 22:02:54]

Description was changed from

==========
[Chromecast] Use base::FeatureList to control features.

In Chromium, Finch-enabled features are controlled through base::FeatureList,
a class which abstracts the experiment framework and developer overrides
from client code. Though Chromecast's experiment framework is fundamentally
different (in that it is server-driven) Cast builds can still make use of
this class. Introduce some utilities to help.

At boot-up, the pref store will be queried for experiment configs, which
were cached to disk on the most recent config fetch from the last boot
cycle. If a developer overrides these features from the command line,
that value takes precedence. These features will be used to initialize
base::FeatureList, which can then be statically queried from any client
code that depends on //base.

This patch does not actually introduce or convert any existing features
to use this framework.

BUG=
==========

to

==========
[Chromecast] Use base::FeatureList to control features.

In Chromium, Finch-enabled features are controlled through base::FeatureList,
a class which abstracts the experiment framework and developer overrides
from client code. Though Chromecast's experiment framework is fundamentally
different (in that it is server-driven) Cast builds can still make use of
this class. Introduce some utilities to help.

At boot-up, the pref store will be queried for experiment configs, which
were cached to disk on the most recent config fetch from the last boot
cycle. If a developer overrides these features from the command line,
that value takes precedence. These features will be used to initialize
base::FeatureList, which can then be statically queried from any client
code that depends on //base.

This patch does not actually introduce or convert any existing features
to use this framework.

BUG=714291, internal b/35424335
==========

[slan, 2017-04-21 22:10:54]

Description was changed from

==========
[Chromecast] Use base::FeatureList to control features.

In Chromium, Finch-enabled features are controlled through base::FeatureList,
a class which abstracts the experiment framework and developer overrides
from client code. Though Chromecast's experiment framework is fundamentally
different (in that it is server-driven) Cast builds can still make use of
this class. Introduce some utilities to help.

At boot-up, the pref store will be queried for experiment configs, which
were cached to disk on the most recent config fetch from the last boot
cycle. If a developer overrides these features from the command line,
that value takes precedence. These features will be used to initialize
base::FeatureList, which can then be statically queried from any client
code that depends on //base.

This patch does not actually introduce or convert any existing features
to use this framework.

BUG=714291, internal b/35424335
==========

to

==========
[Chromecast] Use base::FeatureList to control features.

In Chromium, Finch-enabled features are controlled through base::FeatureList,
a class which abstracts the experiment framework and developer overrides
from client code. Though Chromecast's experiment framework is fundamentally
different (in that it is server-driven) Cast builds can still make use of
this class. Introduce some utilities to help.

At boot-up, the pref store will be queried for experiment configs, which
were cached to disk on the most recent config fetch from the last boot
cycle. If a developer overrides these features from the command line,
that value takes precedence. These features will be used to initialize
base::FeatureList, which can then be statically queried from any client
code that depends on //base.

This patch does not actually introduce or convert any existing features
to use this framework.

BUG=714291
BUG= internal b/35424335
==========

[slan, 2017-04-21 22:11:13]

Thanks!

https://codereview.chromium.org/2825873002/diff/100001/chromecast/base/cast_f...
File chromecast/base/cast_features.cc (right):

https://codereview.chromium.org/2825873002/diff/100001/chromecast/base/cast_f...
chromecast/base/cast_features.cc:71: //     Feature's name as the FieldTrial
name to ensure uniquenes.
On 2017/04/21 21:46:03, Alexei Svitkine (slow) wrote:
> Nit: uniqueness

Done.

[slan, 2017-04-21 22:11:19]

The CQ bit was checked by slan@chromium.org

[slan, 2017-04-21 22:11:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
halliwell@chromium.org, maclellant@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2825873002/#ps120001
(title: "Typo")

[commit-bot: I haz the power, 2017-04-21 22:12:03]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-22 00:21:08]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492812679262780,
"parent_rev": "46b1d72bf455341277bd53b4fc3b404cf7664b98", "commit_rev":
"ef03d2b9a67352094516f4e866373378fb961ee8"}

[commit-bot: I haz the power, 2017-04-22 00:22:18]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492812679262780,
"parent_rev": "46b1d72bf455341277bd53b4fc3b404cf7664b98", "commit_rev":
"ef03d2b9a67352094516f4e866373378fb961ee8"}

[commit-bot: I haz the power, 2017-04-22 00:22:35]

Description was changed from

==========
[Chromecast] Use base::FeatureList to control features.

In Chromium, Finch-enabled features are controlled through base::FeatureList,
a class which abstracts the experiment framework and developer overrides
from client code. Though Chromecast's experiment framework is fundamentally
different (in that it is server-driven) Cast builds can still make use of
this class. Introduce some utilities to help.

At boot-up, the pref store will be queried for experiment configs, which
were cached to disk on the most recent config fetch from the last boot
cycle. If a developer overrides these features from the command line,
that value takes precedence. These features will be used to initialize
base::FeatureList, which can then be statically queried from any client
code that depends on //base.

This patch does not actually introduce or convert any existing features
to use this framework.

BUG=714291
BUG= internal b/35424335
==========

to

==========
[Chromecast] Use base::FeatureList to control features.

In Chromium, Finch-enabled features are controlled through base::FeatureList,
a class which abstracts the experiment framework and developer overrides
from client code. Though Chromecast's experiment framework is fundamentally
different (in that it is server-driven) Cast builds can still make use of
this class. Introduce some utilities to help.

At boot-up, the pref store will be queried for experiment configs, which
were cached to disk on the most recent config fetch from the last boot
cycle. If a developer overrides these features from the command line,
that value takes precedence. These features will be used to initialize
base::FeatureList, which can then be statically queried from any client
code that depends on //base.

This patch does not actually introduce or convert any existing features
to use this framework.

BUG=714291
BUG= internal b/35424335

Review-Url: https://codereview.chromium.org/2825873002
Cr-Commit-Position: refs/heads/master@{#466507}
Committed:
https://chromium.googlesource.com/chromium/src/+/ef03d2b9a67352094516f4e86637...
==========

[commit-bot: I haz the power, 2017-04-22 00:22:36]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ef03d2b9a67352094516f4e86637...

[slan, 2017-04-25 16:06:32]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2838813003/ by slan@chromium.org.

The reason for reverting is: cast_features_browsertest.cc is flaky on our
internal CQ bot - it's unclear why this did not manifest on the Chromium
trybots. 

I assume that state persisting in the PrefService between tests is causing the
flakiness. Reverting this patch until I understand the root cause. .