Changes to support using base/feature_list.h from gin/.

Changes to support using base/feature_list.h from gin/.

Since gin/ (V8) is used from a number of different processes and
test harnesses, this required updating them to ensure they initialize
a base::FeatureList instance (which is required before querying the
API to ensure code doesn't incorrectly query the API too early).

Additionally, for test_suite.cc, the FeatureList initialization needed
to happen earlier - since some sub-classes of it initialize gin from
their Initialize() functions. The initialization was also switched to
use FeatureList::InitializeInstance() since for some test suites,
a FeatureList would already exist.

BUG=599165, 563705
Committed: https://crrev.com/e6be55d0de165a41e25a5bd4d41181f562caf3f3
Cr-Commit-Position: refs/heads/master@{#385053}

[Alexei Svitkine (slow), 2016-04-01 19:34:47]

Patchset #4 (id:60001) has been deleted

[Alexei Svitkine (slow), 2016-04-01 19:34:54]

Patchset #3 (id:40001) has been deleted

[Alexei Svitkine (slow), 2016-04-01 19:34:59]

Patchset #2 (id:20001) has been deleted

[Alexei Svitkine (slow), 2016-04-01 19:35:04]

Patchset #1 (id:1) has been deleted

[Alexei Svitkine (slow), 2016-04-01 19:35:11]

Patchset #3 (id:120001) has been deleted

[Alexei Svitkine (slow), 2016-04-01 19:35:16]

Patchset #2 (id:100001) has been deleted

[Alexei Svitkine (slow), 2016-04-01 19:35:22]

Patchset #1 (id:80001) has been deleted

[Alexei Svitkine (slow), 2016-04-01 20:25:42]

Description was changed from

==========
Changes to support using base/feature_list.h from gin/.

BUG=...
==========

to

==========
Changes to support using base/feature_list.h from gin/.

Since gin/ (V8) is used from a number of different processes and
test harnesses, this required updating them to ensure they initialize
a base::FeatureList instance (which is required before querying the API
to ensure code doesn't incorrectly query the API too early).

BUG=599165,563705
==========

[Alexei Svitkine (slow), 2016-04-01 20:27:54]

Description was changed from

==========
Changes to support using base/feature_list.h from gin/.

Since gin/ (V8) is used from a number of different processes and
test harnesses, this required updating them to ensure they initialize
a base::FeatureList instance (which is required before querying the API
to ensure code doesn't incorrectly query the API too early).

BUG=599165,563705
==========

to

==========
Changes to support using base/feature_list.h from gin/.

Since gin/ (V8) is used from a number of different processes and
test harnesses, this required updating them to ensure they initialize
a base::FeatureList instance (which is required before querying the
API to ensure code doesn't incorrectly query the API too early).

Additionally, for test_suite.cc, the FeatureList initialization needed
to happen earlier - since some sub-classes of it initialize gin from
their Initialize() functions. The initialization was also switched to
use FeatureList::InitializeInstance() since for some test suites,
a FeatureList would already exist.

BUG=599165,563705
==========

[Alexei Svitkine (slow), 2016-04-01 21:17:40]

Patchset #1 (id:140001) has been deleted

[Alexei Svitkine (slow), 2016-04-01 21:19:33]

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

[Alexei Svitkine (slow), 2016-04-01 21:19:34]

rkaplow: FeatureList logic review
jochen: OWNERS review for content/ and gin/

[jochen (gone - plz use gerrit), 2016-04-03 11:17:43]

lgtm

[Alexei Svitkine (slow), 2016-04-04 14:50:57]

asvitkine@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[Alexei Svitkine (slow), 2016-04-04 14:50:57]

Thanks Jochen.

+phajdan.jr for base/test OWNERS

rkaplow, friendly ping for the CL overall. Thanks!

[rkaplow, 2016-04-04 15:46:28]

lgtm

[Paweł Hajdan Jr., 2016-04-04 16:45:17]

LGTM w/optional comment

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

https://codereview.chromium.org/1848523002/diff/180001/base/test/test_suite.c...
base/test/test_suite.cc:215: // Set up a FeatureList instance, so that code
using that API will not hit a
While you're here: could you move this code to Initialize/Shutdown?

Consider this optional, especially if that hits any errors or other yak shave.

However, if just moving it works, I'd recommend we do that.

[Alexei Svitkine (slow), 2016-04-04 20:01:55]

Thanks all! cq-ing

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

https://codereview.chromium.org/1848523002/diff/180001/base/test/test_suite.c...
base/test/test_suite.cc:215: // Set up a FeatureList instance, so that code
using that API will not hit a
On 2016/04/04 16:45:17, Paweł Hajdan Jr. wrote:
> While you're here: could you move this code to Initialize/Shutdown?
> 
> Consider this optional, especially if that hits any errors or other yak shave.
> 
> However, if just moving it works, I'd recommend we do that.

Seems to work. Done.

[Alexei Svitkine (slow), 2016-04-04 20:02:00]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-04-04 20:02:00]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
rkaplow@chromium.org, phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1848523002/#ps240001
(title: "Test passed with gin/isolate_holder.cc feature. Reverting that test
code again.")

[commit-bot: I haz the power, 2016-04-04 20:02:38]

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

[commit-bot: I haz the power, 2016-04-04 21:26:40]

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

[commit-bot: I haz the power, 2016-04-04 21:26:41]

Failed to apply patch for base/test/test_suite.cc:
While running git apply --index -3 -p1;
  error: patch failed: base/test/test_suite.cc:224
  Falling back to three-way merge...
  Applied patch to 'base/test/test_suite.cc' with conflicts.
  U base/test/test_suite.cc

Patch:       base/test/test_suite.cc
Index: base/test/test_suite.cc
diff --git a/base/test/test_suite.cc b/base/test/test_suite.cc
index
74bd9ddf7bd5bd238fb4997630c7dc5bdf51370b..bd3156b4489478ced496ef6c5558efe6909c732c
100644
--- a/base/test/test_suite.cc
+++ b/base/test/test_suite.cc
@@ -98,14 +98,15 @@ int RunUnitTestsUsingBaseTestSuite(int argc, char **argv) {
                          Bind(&TestSuite::Run, Unretained(&test_suite)));
 }
 
-TestSuite::TestSuite(int argc, char** argv) : initialized_command_line_(false)
{
+TestSuite::TestSuite(int argc, char** argv)
+    : initialized_command_line_(false), created_feature_list_(false) {
   PreInitialize();
   InitializeFromCommandLine(argc, argv);
 }
 
 #if defined(OS_WIN)
 TestSuite::TestSuite(int argc, wchar_t** argv)
-    : initialized_command_line_(false) {
+    : initialized_command_line_(false), created_feature_list_(false) {
   PreInitialize();
   InitializeFromCommandLine(argc, argv);
 }
@@ -224,15 +225,8 @@ int TestSuite::Run() {
   test_listener_ios::RegisterTestEndListener();
 #endif
 
-  // Set up a FeatureList instance, so that code using that API will not hit a
-  // an error that it's not set. Cleared by ClearInstanceForTesting() below.
-  base::FeatureList::SetInstance(make_scoped_ptr(new base::FeatureList));
-
   int result = RUN_ALL_TESTS();
 
-  // Clear the FeatureList that was registered above.
-  FeatureList::ClearInstanceForTesting();
-
 #if defined(OS_MACOSX)
   // This MUST happen before Shutdown() since Shutdown() tears down
   // objects (such as NotificationService::current()) that Cocoa
@@ -294,6 +288,13 @@ void TestSuite::Initialize() {
   }
 #endif
 
+  // Set up a FeatureList instance, so that code using that API will not hit a
+  // an error that it's not set. If a FeatureList was created in this way (i.e.
+  // one didn't exist previously), it will be cleared in Shutdown() via
+  // ClearInstanceForTesting().
+  created_feature_list_ =
+      FeatureList::InitializeInstance(std::string(), std::string());
+
 #if defined(OS_IOS)
   InitIOSTestMessageLoop();
 #endif  // OS_IOS
@@ -362,6 +363,9 @@ void TestSuite::Initialize() {
 }
 
 void TestSuite::Shutdown() {
+  // Clear the FeatureList that was created by Initialize().
+  if (created_feature_list_)
+    FeatureList::ClearInstanceForTesting();
 }
 
 }  // namespace base

[commit-bot: I haz the power, 2016-04-04 21:27:08]

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

[Alexei Svitkine (slow), 2016-04-04 21:37:40]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-04-04 21:37:40]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
rkaplow@chromium.org, phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1848523002/#ps260001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-04-04 21:38:07]

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

[commit-bot: I haz the power, 2016-04-04 23:29:59]

Description was changed from

==========
Changes to support using base/feature_list.h from gin/.

Since gin/ (V8) is used from a number of different processes and
test harnesses, this required updating them to ensure they initialize
a base::FeatureList instance (which is required before querying the
API to ensure code doesn't incorrectly query the API too early).

Additionally, for test_suite.cc, the FeatureList initialization needed
to happen earlier - since some sub-classes of it initialize gin from
their Initialize() functions. The initialization was also switched to
use FeatureList::InitializeInstance() since for some test suites,
a FeatureList would already exist.

BUG=599165,563705
==========

to

==========
Changes to support using base/feature_list.h from gin/.

Since gin/ (V8) is used from a number of different processes and
test harnesses, this required updating them to ensure they initialize
a base::FeatureList instance (which is required before querying the
API to ensure code doesn't incorrectly query the API too early).

Additionally, for test_suite.cc, the FeatureList initialization needed
to happen earlier - since some sub-classes of it initialize gin from
their Initialize() functions. The initialization was also switched to
use FeatureList::InitializeInstance() since for some test suites,
a FeatureList would already exist.

BUG=599165,563705
==========

[commit-bot: I haz the power, 2016-04-04 23:30:02]

Committed patchset #6 (id:260001)

[commit-bot: I haz the power, 2016-04-04 23:31:55]

Description was changed from

==========
Changes to support using base/feature_list.h from gin/.

Since gin/ (V8) is used from a number of different processes and
test harnesses, this required updating them to ensure they initialize
a base::FeatureList instance (which is required before querying the
API to ensure code doesn't incorrectly query the API too early).

Additionally, for test_suite.cc, the FeatureList initialization needed
to happen earlier - since some sub-classes of it initialize gin from
their Initialize() functions. The initialization was also switched to
use FeatureList::InitializeInstance() since for some test suites,
a FeatureList would already exist.

BUG=599165,563705
==========

to

==========
Changes to support using base/feature_list.h from gin/.

Since gin/ (V8) is used from a number of different processes and
test harnesses, this required updating them to ensure they initialize
a base::FeatureList instance (which is required before querying the
API to ensure code doesn't incorrectly query the API too early).

Additionally, for test_suite.cc, the FeatureList initialization needed
to happen earlier - since some sub-classes of it initialize gin from
their Initialize() functions. The initialization was also switched to
use FeatureList::InitializeInstance() since for some test suites,
a FeatureList would already exist.

BUG=599165,563705

Committed: https://crrev.com/e6be55d0de165a41e25a5bd4d41181f562caf3f3
Cr-Commit-Position: refs/heads/master@{#385053}
==========

[commit-bot: I haz the power, 2016-04-04 23:31:57]

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