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

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

This feature was reverted due to the new browsertest being flaky on
internal Cast infrastructure: crrev.com/2838813003

=== Original Commit Message ===
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/2836263003
Cr-Commit-Position: refs/heads/master@{#467998}
Committed: https://chromium.googlesource.com/chromium/src/+/aab6bf24d32d931359b2d0c25fb39b3ed51fbc86

[slan, 2017-04-26 20:33:52]

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

[slan, 2017-04-26 20:33:52]

The browsertests were flaking b/c there were implicit state dependencies between
tests. I have removed those implicit dependencies, and added guidance for future
development.

PS1 is the orginal, unaltered patch.
PS2 has the browsertest improvements.

PTAL.

[slan, 2017-04-26 20:34:49]

(PS2 also adds a test for experiment ids)

[slan, 2017-04-26 21:00:12]

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

[commit-bot: I haz the power, 2017-04-26 21:00:58]

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-26 22:39:36]

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

[commit-bot: I haz the power, 2017-04-26 22:39:37]

Dry run: This issue passed the CQ dry run.

[halliwell, 2017-04-27 00:04:23]

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

https://codereview.chromium.org/2836263003/diff/20001/chromecast/browser/test...
chromecast/browser/test/cast_features_browsertest.cc:46: const base::Feature
kEnableBat("enable_bat", base::FEATURE_ENABLED_BY_DEFAULT);
nit: would it make sense to have a more consistent naming convention for these
groups of features?

https://codereview.chromium.org/2836263003/diff/20001/chromecast/browser/test...
chromecast/browser/test/cast_features_browsertest.cc:74: // If the test failed,
scrub any state from prefs.
What is this block for exactly?  does it cover clearing state on disk?  What
happens if tests crash (i.e. failure that won't run this cleanup)?  Does it make
more sense to clean everything up at the start (I think there's a setup function
that's called once before all tests run)?

https://codereview.chromium.org/2836263003/diff/20001/chromecast/browser/test...
chromecast/browser/test/cast_features_browsertest.cc:99:
DCHECK(!features_declared_);
here and below, should these be asserts running on all build flavours?

https://codereview.chromium.org/2836263003/diff/20001/chromecast/browser/test...
chromecast/browser/test/cast_features_browsertest.cc:105: // DelcareFeatures()
above). DeclareFeatures() must be called before this
DeclareFeatures

[slan, 2017-04-27 15:36:02]

Thanks for comments, Luke. PTAL.

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

https://codereview.chromium.org/2836263003/diff/20001/chromecast/browser/test...
chromecast/browser/test/cast_features_browsertest.cc:46: const base::Feature
kEnableBat("enable_bat", base::FEATURE_ENABLED_BY_DEFAULT);
On 2017/04/27 00:04:23, halliwell wrote:
> nit: would it make sense to have a more consistent naming convention for these
> groups of features?

Made it a little more formulaic, added guidance.

https://codereview.chromium.org/2836263003/diff/20001/chromecast/browser/test...
chromecast/browser/test/cast_features_browsertest.cc:74: // If the test failed,
scrub any state from prefs.
On 2017/04/27 00:04:23, halliwell wrote:
> What is this block for exactly?  does it cover clearing state on disk?  What
> happens if tests crash (i.e. failure that won't run this cleanup)?  Does it
make
> more sense to clean everything up at the start (I think there's a setup
function
> that's called once before all tests run)?

All really good points. On crash, the device would be left in a bad state.

Spent about 30 minutes trying to make SetUpTestCase (the method to which you
refer) work for us here, but couldn't make it work. We need too many things from
the browser (mostly the BrowserThread and the PathService) to effectively modify
the PrefStore in a non-hacky way, and the browser is not set up at that point.

So... I decided to go a little more low-tech and add a PRE_PRE_* test to each
group which clears the set of features used by each test. I also added some
guidance up top. This allows us to remove any of the declared feature nonsense,
and always ensures that the test run is good, no matter the state of the device.

https://codereview.chromium.org/2836263003/diff/20001/chromecast/browser/test...
chromecast/browser/test/cast_features_browsertest.cc:99:
DCHECK(!features_declared_);
On 2017/04/27 00:04:23, halliwell wrote:
> here and below, should these be asserts running on all build flavours?

No more DCHECKS after changes above.

https://codereview.chromium.org/2836263003/diff/20001/chromecast/browser/test...
chromecast/browser/test/cast_features_browsertest.cc:105: // DelcareFeatures()
above). DeclareFeatures() must be called before this
On 2017/04/27 00:04:23, halliwell wrote:
> DeclareFeatures

Irrelevant

[halliwell, 2017-04-27 18:12:00]

On 2017/04/27 15:36:02, slan wrote:
> Thanks for comments, Luke. PTAL.
> 
>
https://codereview.chromium.org/2836263003/diff/20001/chromecast/browser/test...
> File chromecast/browser/test/cast_features_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2836263003/diff/20001/chromecast/browser/test...
> chromecast/browser/test/cast_features_browsertest.cc:46: const base::Feature
> kEnableBat("enable_bat", base::FEATURE_ENABLED_BY_DEFAULT);
> On 2017/04/27 00:04:23, halliwell wrote:
> > nit: would it make sense to have a more consistent naming convention for
these
> > groups of features?
> 
> Made it a little more formulaic, added guidance.
> 
>
https://codereview.chromium.org/2836263003/diff/20001/chromecast/browser/test...
> chromecast/browser/test/cast_features_browsertest.cc:74: // If the test
failed,
> scrub any state from prefs.
> On 2017/04/27 00:04:23, halliwell wrote:
> > What is this block for exactly?  does it cover clearing state on disk?  What
> > happens if tests crash (i.e. failure that won't run this cleanup)?  Does it
> make
> > more sense to clean everything up at the start (I think there's a setup
> function
> > that's called once before all tests run)?
> 
> All really good points. On crash, the device would be left in a bad state.
> 
> Spent about 30 minutes trying to make SetUpTestCase (the method to which you
> refer) work for us here, but couldn't make it work. We need too many things
from
> the browser (mostly the BrowserThread and the PathService) to effectively
modify
> the PrefStore in a non-hacky way, and the browser is not set up at that point.
> 
> So... I decided to go a little more low-tech and add a PRE_PRE_* test to each
> group which clears the set of features used by each test. I also added some
> guidance up top. This allows us to remove any of the declared feature
nonsense,
> and always ensures that the test run is good, no matter the state of the
device.
> 
>
https://codereview.chromium.org/2836263003/diff/20001/chromecast/browser/test...
> chromecast/browser/test/cast_features_browsertest.cc:99:
> DCHECK(!features_declared_);
> On 2017/04/27 00:04:23, halliwell wrote:
> > here and below, should these be asserts running on all build flavours?
> 
> No more DCHECKS after changes above.
> 
>
https://codereview.chromium.org/2836263003/diff/20001/chromecast/browser/test...
> chromecast/browser/test/cast_features_browsertest.cc:105: // DelcareFeatures()
> above). DeclareFeatures() must be called before this
> On 2017/04/27 00:04:23, halliwell wrote:
> > DeclareFeatures
> 
> Irrelevant

PRE_PRE, nice :)  lgtm!

[slan, 2017-04-27 18:15:25]

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

[slan, 2017-04-27 18:15:45]

On 2017/04/27 18:12:00, halliwell wrote:
> On 2017/04/27 15:36:02, slan wrote:
> > Thanks for comments, Luke. PTAL.
> > 
> >
>
https://codereview.chromium.org/2836263003/diff/20001/chromecast/browser/test...
> > File chromecast/browser/test/cast_features_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2836263003/diff/20001/chromecast/browser/test...
> > chromecast/browser/test/cast_features_browsertest.cc:46: const base::Feature
> > kEnableBat("enable_bat", base::FEATURE_ENABLED_BY_DEFAULT);
> > On 2017/04/27 00:04:23, halliwell wrote:
> > > nit: would it make sense to have a more consistent naming convention for
> these
> > > groups of features?
> > 
> > Made it a little more formulaic, added guidance.
> > 
> >
>
https://codereview.chromium.org/2836263003/diff/20001/chromecast/browser/test...
> > chromecast/browser/test/cast_features_browsertest.cc:74: // If the test
> failed,
> > scrub any state from prefs.
> > On 2017/04/27 00:04:23, halliwell wrote:
> > > What is this block for exactly?  does it cover clearing state on disk? 
What
> > > happens if tests crash (i.e. failure that won't run this cleanup)?  Does
it
> > make
> > > more sense to clean everything up at the start (I think there's a setup
> > function
> > > that's called once before all tests run)?
> > 
> > All really good points. On crash, the device would be left in a bad state.
> > 
> > Spent about 30 minutes trying to make SetUpTestCase (the method to which you
> > refer) work for us here, but couldn't make it work. We need too many things
> from
> > the browser (mostly the BrowserThread and the PathService) to effectively
> modify
> > the PrefStore in a non-hacky way, and the browser is not set up at that
point.
> > 
> > So... I decided to go a little more low-tech and add a PRE_PRE_* test to
each
> > group which clears the set of features used by each test. I also added some
> > guidance up top. This allows us to remove any of the declared feature
> nonsense,
> > and always ensures that the test run is good, no matter the state of the
> device.
> > 
> >
>
https://codereview.chromium.org/2836263003/diff/20001/chromecast/browser/test...
> > chromecast/browser/test/cast_features_browsertest.cc:99:
> > DCHECK(!features_declared_);
> > On 2017/04/27 00:04:23, halliwell wrote:
> > > here and below, should these be asserts running on all build flavours?
> > 
> > No more DCHECKS after changes above.
> > 
> >
>
https://codereview.chromium.org/2836263003/diff/20001/chromecast/browser/test...
> > chromecast/browser/test/cast_features_browsertest.cc:105: //
DelcareFeatures()
> > above). DeclareFeatures() must be called before this
> > On 2017/04/27 00:04:23, halliwell wrote:
> > > DeclareFeatures
> > 
> > Irrelevant
> 
> PRE_PRE, nice :)  lgtm!

Will run once more against the internal bots, then submit if OK.

[commit-bot: I haz the power, 2017-04-27 18:15:55]

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-27 18:36:49]

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

[commit-bot: I haz the power, 2017-04-27 18:36:50]

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

[slan, 2017-04-28 13:41:28]

The CQ bit was checked by slan@chromium.org

[commit-bot: I haz the power, 2017-04-28 13:41:50]

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-28 15:09:43]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493386888901220,
"parent_rev": "c007949541f4d2238c9653a4e3b8dbc0987bc752", "commit_rev":
"aab6bf24d32d931359b2d0c25fb39b3ed51fbc86"}

[commit-bot: I haz the power, 2017-04-28 15:09:53]

Description was changed from

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

This feature was reverted due to the new browsertest being flaky on
internal Cast infrastructure: crrev.com/2838813003

=== Original Commit Message ===
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

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

This feature was reverted due to the new browsertest being flaky on
internal Cast infrastructure: crrev.com/2838813003

=== Original Commit Message ===
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/2836263003
Cr-Commit-Position: refs/heads/master@{#467998}
Committed:
https://chromium.googlesource.com/chromium/src/+/aab6bf24d32d931359b2d0c25fb3...
==========

[commit-bot: I haz the power, 2017-04-28 15:09:54]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/aab6bf24d32d931359b2d0c25fb3...

[slan, 2017-04-29 20:02:38]

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

The reason for reverting is: Breaks ATV at runtime..