Reland of Change ScopedFeatureList to overrides FeatureList not reset

Reland of Change ScopedFeatureList to overrides FeatureList not reset

The current situation is that using ScopedFeatureList resets to an
empty feature list and then enables/disables an explicit list of
features.

That's never what you want for browser tests (or other higher-level
tests) since it effectively overrides higher-level test configurations
(e.g. those in fieldtrial_testing_config.json, or a bot set up to
specifically test a feature).

In this patch:

1. Keep SFL::Init, SFL::InitWithFeatureList,
   SFL::InitFromCommandLine reset to empty list but add warning to
   remind developer should use them with care.
2. Change SFL::InitAndEnableFeature, SFL::InitAndDisableFeature and
   SFL::InitWithFeatures to not reset but override current FeatureList
   with given enables/disables.

We also add unit tests for ScopedFeatureList.

BUG=713390
Original-Review-Url: https://codereview.chromium.org/2834583002
Review-Url: https://codereview.chromium.org/2968793002
Cr-Commit-Position: refs/heads/master@{#484921}
Committed: https://chromium.googlesource.com/chromium/src/+/e86c74ee8e06e2a7538b273efe5e4f17fc0c38f9

[chaopeng, 2017-06-30 19:34:18]

Created Reland of Change ScopedFeatureList to overrides FeatureList not reset

[chaopeng, 2017-07-01 00:04:32]

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

[commit-bot: I haz the power, 2017-07-01 00:04:52]

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-07-01 00:04:54]

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

[commit-bot: I haz the power, 2017-07-01 00:04:55]

Dry run: This issue passed the CQ dry run.

[chaopeng, 2017-07-01 00:20:50]

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

[commit-bot: I haz the power, 2017-07-01 00:21:05]

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-07-01 00:21:06]

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

[commit-bot: I haz the power, 2017-07-01 00:21:07]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-07-03 02:32:42]

LGTM

[Lei Zhang, 2017-07-05 18:31:50]

lgtm, but can you write a proper CL description? (AKA commit message) The revert
of a revert of a revert is getting out of control.

Also, did this CL change between its latest patch set and the last attempt to
land it? Looks like no, so feel free to TBR.

[chaopeng, 2017-07-05 21:22:45]

Description was changed from

==========
Reland of Change ScopedFeatureList to overrides FeatureList not reset (patchset
#1 id:1 of https://codereview.chromium.org/2964623002/ )

Reason for revert:
OOM issue addressed. Ready for reland.

Original issue's description:
> Revert of Change ScopedFeatureList to overrides FeatureList not reset
(patchset #1 id:1 of https://codereview.chromium.org/2962963002/ )
> 
> Reason for revert:
> Causing Mac bots to OOM on AppShimInteractiveTest tests
> 
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.mac%2FMac10.10_...
> 
> Original issue's description:
> > Reland of Change ScopedFeatureList to overrides FeatureList not reset
(patchset #1 id:1 of https://codereview.chromium.org/2850073002/ )
> > 
> > Reason for revert:
> > The ASAN failure is fixed at https://codereview.chromium.org/2874693002/. 
> > 
> > This patch is ready for reland.
> > 
> > Original issue's description:
> > > Revert of Change ScopedFeatureList to overrides FeatureList not reset
(patchset #6 id:140001 of https://codereview.chromium.org/2834583002/ )
> > > 
> > > Reason for revert:
> > > Mac ASAN bots reporting use-after-free errors.
> > > 
> > > Original issue's description:
> > > > Change ScopedFeatureList to overrides FeatureList not reset
> > > > 
> > > > The current situation is that using ScopedFeatureList resets to an
> > > > empty feature list and then enables/disables an explicit list of
> > > > features.
> > > > 
> > > > That's never what you want for browser tests (or other higher-level
> > > > tests) since it effectively overrides higher-level test configurations
> > > > (e.g. those in fieldtrial_testing_config.json, or a bot set up to
> > > > specifically test a feature).
> > > > 
> > > > In this patch:
> > > > 
> > > > 1. Keep SFL::Init, SFL::InitWithFeatureList,
> > > >    SFL::InitFromCommandLine reset to empty list but add warning to
> > > >    remind developer should use them with care.
> > > > 2. Change SFL::InitAndEnableFeature, SFL::InitAndDisableFeature and
> > > >    SFL::InitWithFeatures to not reset but override current FeatureList
> > > >    with given enables/disables.
> > > > 
> > > > We also add unit tests for ScopedFeatureList.
> > > > 
> > > > BUG=713390
> > > > 
> > > > Review-Url: https://codereview.chromium.org/2834583002
> > > > Cr-Commit-Position: refs/heads/master@{#468210}
> > > > Committed:
https://chromium.googlesource.com/chromium/src/+/9c04ed553bd7abe820a6a93c5e89...
> > > 
> > >
TBR=isherman@chromium.org,asvitkine@chromium.org,thakis@chromium.org,chaopeng...
> > > # Not skipping CQ checks because original CL landed more than 1 days ago.
> > > BUG=713390
> > > 
> > > Review-Url: https://codereview.chromium.org/2850073002
> > > Cr-Commit-Position: refs/heads/master@{#468263}
> > > Committed:
https://chromium.googlesource.com/chromium/src/+/6c14f269fb196627c72a81020548...
> > 
> >
TBR=isherman@chromium.org,asvitkine@chromium.org,thakis@chromium.org,thestig@...
> > # Not skipping CQ checks because original CL landed more than 1 days ago.
> > BUG=713390
> > 
> > Review-Url: https://codereview.chromium.org/2962963002
> > Cr-Commit-Position: refs/heads/master@{#483631}
> > Committed:
https://chromium.googlesource.com/chromium/src/+/04afdfd65fe04ef71fa9918d8fea...
> 
>
TBR=isherman@chromium.org,asvitkine@chromium.org,thakis@chromium.org,thestig@...
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=713390
> 
> Review-Url: https://codereview.chromium.org/2964623002
> Cr-Commit-Position: refs/heads/master@{#483641}
> Committed:
https://chromium.googlesource.com/chromium/src/+/cdb47416618522534d42103efe54...

TBR=isherman@chromium.org,asvitkine@chromium.org,thakis@chromium.org,thestig@...
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=713390
==========

to

==========
Reland of Change ScopedFeatureList to overrides FeatureList not reset

The current situation is that using ScopedFeatureList resets to an
empty feature list and then enables/disables an explicit list of
features.

That's never what you want for browser tests (or other higher-level
tests) since it effectively overrides higher-level test configurations
(e.g. those in fieldtrial_testing_config.json, or a bot set up to
specifically test a feature).

In this patch:

1. Keep SFL::Init, SFL::InitWithFeatureList,
   SFL::InitFromCommandLine reset to empty list but add warning to
   remind developer should use them with care.
2. Change SFL::InitAndEnableFeature, SFL::InitAndDisableFeature and
   SFL::InitWithFeatures to not reset but override current FeatureList
   with given enables/disables.

We also add unit tests for ScopedFeatureList.

BUG=713390
Original-Review-Url: https://codereview.chromium.org/2834583002
==========

[chaopeng, 2017-07-06 19:29:46]

The CQ bit was checked by chaopeng@chromium.org

[commit-bot: I haz the power, 2017-07-06 19:30:07]

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-07-06 20:07:46]

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

[commit-bot: I haz the power, 2017-07-06 20:07:47]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[chaopeng, 2017-07-07 01:09:17]

The CQ bit was checked by chaopeng@chromium.org

[commit-bot: I haz the power, 2017-07-07 01:09:27]

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-07-07 02:14:54]

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

[commit-bot: I haz the power, 2017-07-07 02:14:56]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[chaopeng, 2017-07-07 13:51:35]

The CQ bit was checked by chaopeng@chromium.org

[commit-bot: I haz the power, 2017-07-07 13:51:49]

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-07-07 14:44:19]

CQ is committing da patch.

Bot data: {"patchset_id": 130001, "attempt_start_ts": 1499435495276710,
"parent_rev": "2f98242f3b670cafd1f6712143ed9306364376fc", "commit_rev":
"0c5ccc2eb3ff8695695feea2209be086d916f0da"}

[commit-bot: I haz the power, 2017-07-07 14:44:36]

CQ is committing da patch.

Bot data: {"patchset_id": 130001, "attempt_start_ts": 1499435495276710,
"parent_rev": "db92e6b421f4d3e40a01a00a0fc77b9ad2fe13af", "commit_rev":
"e86c74ee8e06e2a7538b273efe5e4f17fc0c38f9"}

[commit-bot: I haz the power, 2017-07-07 14:44:52]

Description was changed from

==========
Reland of Change ScopedFeatureList to overrides FeatureList not reset

The current situation is that using ScopedFeatureList resets to an
empty feature list and then enables/disables an explicit list of
features.

That's never what you want for browser tests (or other higher-level
tests) since it effectively overrides higher-level test configurations
(e.g. those in fieldtrial_testing_config.json, or a bot set up to
specifically test a feature).

In this patch:

1. Keep SFL::Init, SFL::InitWithFeatureList,
   SFL::InitFromCommandLine reset to empty list but add warning to
   remind developer should use them with care.
2. Change SFL::InitAndEnableFeature, SFL::InitAndDisableFeature and
   SFL::InitWithFeatures to not reset but override current FeatureList
   with given enables/disables.

We also add unit tests for ScopedFeatureList.

BUG=713390
Original-Review-Url: https://codereview.chromium.org/2834583002
==========

to

==========
Reland of Change ScopedFeatureList to overrides FeatureList not reset

The current situation is that using ScopedFeatureList resets to an
empty feature list and then enables/disables an explicit list of
features.

That's never what you want for browser tests (or other higher-level
tests) since it effectively overrides higher-level test configurations
(e.g. those in fieldtrial_testing_config.json, or a bot set up to
specifically test a feature).

In this patch:

1. Keep SFL::Init, SFL::InitWithFeatureList,
   SFL::InitFromCommandLine reset to empty list but add warning to
   remind developer should use them with care.
2. Change SFL::InitAndEnableFeature, SFL::InitAndDisableFeature and
   SFL::InitWithFeatures to not reset but override current FeatureList
   with given enables/disables.

We also add unit tests for ScopedFeatureList.

BUG=713390
Original-Review-Url: https://codereview.chromium.org/2834583002
Review-Url: https://codereview.chromium.org/2968793002
Cr-Commit-Position: refs/heads/master@{#484921}
Committed:
https://chromium.googlesource.com/chromium/src/+/e86c74ee8e06e2a7538b273efe5e...
==========

[commit-bot: I haz the power, 2017-07-07 14:44:53]

Committed patchset #2 (id:130001) as
https://chromium.googlesource.com/chromium/src/+/e86c74ee8e06e2a7538b273efe5e...