Add a ScopedFeatureList class for testing and start using it.

Add a ScopedFeatureList class for testing and start using it.

This class improves upon the current situation by ensuring a test
doesn't end up modifying the global feature list permanently.

Migrates a number of tests to new the scoped class.

BUG=620435
TBR=jochen@chromium.org

Committed: https://crrev.com/9499b8d28ace546ebe511b777fa3529421ecc256
Cr-Commit-Position: refs/heads/master@{#410589}

[Alexei Svitkine (slow), 2016-08-08 21:19:54]

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

[commit-bot: I haz the power, 2016-08-08 21:20:19]

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

[Alexei Svitkine (slow), 2016-08-08 21:48:40]

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

[commit-bot: I haz the power, 2016-08-08 21:49:07]

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, 2016-08-08 22:07:59]

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

[commit-bot: I haz the power, 2016-08-08 22:08:00]

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

[Alexei Svitkine (slow), 2016-08-08 22:30:49]

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

[commit-bot: I haz the power, 2016-08-08 22:31:31]

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

[Alexei Svitkine (slow), 2016-08-09 00:48:55]

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

[Alexei Svitkine (slow), 2016-08-09 00:48:56]

isherman: PTAL for the feature list code in base/ and base/test

[commit-bot: I haz the power, 2016-08-09 00:54:45]

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

[commit-bot: I haz the power, 2016-08-09 00:54:46]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2016-08-09 01:06:57]

Awesome!  LGTM % nits.

https://codereview.chromium.org/2226063002/diff/60001/base/feature_list.h
File base/feature_list.h (right):

https://codereview.chromium.org/2226063002/diff/60001/base/feature_list.h#new...
base/feature_list.h:170: // Note: Most tests should never call this directly,
instead consider using
nit: s/directly, instead/directly. Instead

https://codereview.chromium.org/2226063002/diff/60001/base/feature_list.h#new...
base/feature_list.h:175: // list, for testing. Existing instance must be null.
This is primarily
nit: You use the word "existing" in two different ways here -- please rephrase
one of them.

https://codereview.chromium.org/2226063002/diff/60001/base/test/scoped_featur...
File base/test/scoped_feature_list.h (right):

https://codereview.chromium.org/2226063002/diff/60001/base/test/scoped_featur...
base/test/scoped_feature_list.h:24: void Init();
Optional nit: I think it would be cleaner if each of the Init methods were a
constructor -- that way, code doesn't need to first construct the object, and
then init it.

https://codereview.chromium.org/2226063002/diff/60001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/2226063002/diff/60001/base/test/test_suite.cc...
base/test/test_suite.cc:314: printf("initing the instance\n");
Plz revert ;)

https://codereview.chromium.org/2226063002/diff/60001/components/password_man...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/2226063002/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager_unittest.cc:772:
scoped_feature_list_.InitAndEnableFeature(features::kFillOnAccountSelect);
nit: Would be better to do this in the constructor, and remove the SetUp()
method.

[Alexei Svitkine (slow), 2016-08-09 01:26:16]

https://codereview.chromium.org/2226063002/diff/60001/base/feature_list.h
File base/feature_list.h (right):

https://codereview.chromium.org/2226063002/diff/60001/base/feature_list.h#new...
base/feature_list.h:170: // Note: Most tests should never call this directly,
instead consider using
On 2016/08/09 01:06:57, Ilya Sherman wrote:
> nit: s/directly, instead/directly. Instead

Done.

https://codereview.chromium.org/2226063002/diff/60001/base/feature_list.h#new...
base/feature_list.h:175: // list, for testing. Existing instance must be null.
This is primarily
On 2016/08/09 01:06:57, Ilya Sherman wrote:
> nit: You use the word "existing" in two different ways here -- please rephrase
> one of them.

Done.

https://codereview.chromium.org/2226063002/diff/60001/base/test/scoped_featur...
File base/test/scoped_feature_list.h (right):

https://codereview.chromium.org/2226063002/diff/60001/base/test/scoped_featur...
base/test/scoped_feature_list.h:24: void Init();
On 2016/08/09 01:06:57, Ilya Sherman wrote:
> Optional nit: I think it would be cleaner if each of the Init methods were a
> constructor -- that way, code doesn't need to first construct the object, and
> then init it.

That doesn't fit some use cases - where e.g. the scoped feature list is a member
of a test harness class and then *some* tests but not all initialize it to some
given state. I'll leave it as-is for now, but we can consider also adding the
ctor params as a short hand syntax for not calling the Init functions.

https://codereview.chromium.org/2226063002/diff/60001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/2226063002/diff/60001/base/test/test_suite.cc...
base/test/test_suite.cc:314: printf("initing the instance\n");
On 2016/08/09 01:06:57, Ilya Sherman wrote:
> Plz revert ;)

Done.

https://codereview.chromium.org/2226063002/diff/60001/components/password_man...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/2226063002/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager_unittest.cc:772:
scoped_feature_list_.InitAndEnableFeature(features::kFillOnAccountSelect);
On 2016/08/09 01:06:57, Ilya Sherman wrote:
> nit: Would be better to do this in the constructor, and remove the SetUp()
> method.

Done.

[Alexei Svitkine (slow), 2016-08-09 01:29:57]

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

[Alexei Svitkine (slow), 2016-08-09 01:29:57]

+TBR jochen@chromium.org for downstream owners. TBRing since this is essentially
an API change.

[Alexei Svitkine (slow), 2016-08-09 01:30:29]

Description was changed from

==========
Add a ScopedFeatureList class for testing and start using it.

This class improves upon the current situation by ensuring a test
doesn't end up modifying the global feature list permanently.

Migrates a number of tests to new the scoped class.

BUG=620435
==========

to

==========
Add a ScopedFeatureList class for testing and start using it.

This class improves upon the current situation by ensuring a test
doesn't end up modifying the global feature list permanently.

Migrates a number of tests to new the scoped class.

BUG=620435
TBR=jochen@chromium.org
==========

[Alexei Svitkine (slow), 2016-08-09 01:31:49]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-08-09 01:31:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2226063002/#ps80001
(title: "Addressed comments.")

[commit-bot: I haz the power, 2016-08-09 01:32:26]

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, 2016-08-09 01:41:09]

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

[commit-bot: I haz the power, 2016-08-09 01:41:10]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Alexei Svitkine (slow), 2016-08-09 02:44:50]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2016-08-09 02:45:23]

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

[Alexei Svitkine (slow), 2016-08-09 02:57:49]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-08-09 02:57:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2226063002/#ps100001
(title: "Fix issue in previous patchset.")

[commit-bot: I haz the power, 2016-08-09 02:58:09]

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, 2016-08-09 05:37:25]

Description was changed from

==========
Add a ScopedFeatureList class for testing and start using it.

This class improves upon the current situation by ensuring a test
doesn't end up modifying the global feature list permanently.

Migrates a number of tests to new the scoped class.

BUG=620435
TBR=jochen@chromium.org
==========

to

==========
Add a ScopedFeatureList class for testing and start using it.

This class improves upon the current situation by ensuring a test
doesn't end up modifying the global feature list permanently.

Migrates a number of tests to new the scoped class.

BUG=620435
TBR=jochen@chromium.org
==========

[commit-bot: I haz the power, 2016-08-09 05:37:27]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-09 05:40:29]

Description was changed from

==========
Add a ScopedFeatureList class for testing and start using it.

This class improves upon the current situation by ensuring a test
doesn't end up modifying the global feature list permanently.

Migrates a number of tests to new the scoped class.

BUG=620435
TBR=jochen@chromium.org
==========

to

==========
Add a ScopedFeatureList class for testing and start using it.

This class improves upon the current situation by ensuring a test
doesn't end up modifying the global feature list permanently.

Migrates a number of tests to new the scoped class.

BUG=620435
TBR=jochen@chromium.org

Committed: https://crrev.com/9499b8d28ace546ebe511b777fa3529421ecc256
Cr-Commit-Position: refs/heads/master@{#410589}
==========

[commit-bot: I haz the power, 2016-08-09 05:40:31]

Patchset 6 (id:??) landed as
https://crrev.com/9499b8d28ace546ebe511b777fa3529421ecc256
Cr-Commit-Position: refs/heads/master@{#410589}

[Ilya Sherman, 2016-08-09 06:15:12]

https://codereview.chromium.org/2226063002/diff/60001/base/test/scoped_featur...
File base/test/scoped_feature_list.h (right):

https://codereview.chromium.org/2226063002/diff/60001/base/test/scoped_featur...
base/test/scoped_feature_list.h:24: void Init();
On 2016/08/09 01:26:16, Alexei Svitkine (very slow) wrote:
> On 2016/08/09 01:06:57, Ilya Sherman wrote:
> > Optional nit: I think it would be cleaner if each of the Init methods were a
> > constructor -- that way, code doesn't need to first construct the object,
and
> > then init it.
> 
> That doesn't fit some use cases - where e.g. the scoped feature list is a
member
> of a test harness class and then *some* tests but not all initialize it to
some
> given state. I'll leave it as-is for now, but we can consider also adding the
> ctor params as a short hand syntax for not calling the Init functions.

FWIW, my preferred solution to that would be for the individual tests to declare
the scoped feature list, rather than having it be a member of the test harness
class.

[vabr (Chromium), 2016-08-09 07:04:24]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2016-08-09 07:04:25]

You don't need my approval, but I had to say that this LGTM. In fact, thanks a
lot for this awesome change! :)

Vaclav

[Alexei Svitkine (slow), 2016-08-09 15:27:43]

https://codereview.chromium.org/2226063002/diff/60001/base/test/scoped_featur...
File base/test/scoped_feature_list.h (right):

https://codereview.chromium.org/2226063002/diff/60001/base/test/scoped_featur...
base/test/scoped_feature_list.h:24: void Init();
On 2016/08/09 06:15:12, Ilya Sherman wrote:
> On 2016/08/09 01:26:16, Alexei Svitkine (very slow) wrote:
> > On 2016/08/09 01:06:57, Ilya Sherman wrote:
> > > Optional nit: I think it would be cleaner if each of the Init methods were
a
> > > constructor -- that way, code doesn't need to first construct the object,
> and
> > > then init it.
> > 
> > That doesn't fit some use cases - where e.g. the scoped feature list is a
> member
> > of a test harness class and then *some* tests but not all initialize it to
> some
> > given state. I'll leave it as-is for now, but we can consider also adding
the
> > ctor params as a short hand syntax for not calling the Init functions.
> 
> FWIW, my preferred solution to that would be for the individual tests to
declare
> the scoped feature list, rather than having it be a member of the test harness
> class.

Okay, I can take a look at it when I plan to do my second pass in converting
existing tests, probably when I'll be traveling Thursday next week.