Support Using ScopedFeatureList in BrowserTest

content/public/test/browser_test_base.cc

Index: content/public/test/browser_test_base.cc
diff --git a/content/public/test/browser_test_base.cc b/content/public/test/browser_test_base.cc
index 9526a3927e82bbcaf0e6bcdea5e131530abbcb63..17b0224b00bb5c4393a927e020cace23265d6853 100644
--- a/content/public/test/browser_test_base.cc
+++ b/content/public/test/browser_test_base.cc
@@ -17,6 +17,7 @@
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/sys_info.h"
+#include "base/test/scoped_command_line.h"
 #include "base/test/test_timeouts.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/threading/thread_restrictions.h"
@@ -151,7 +152,8 @@ void BrowserTestBase::SetUp() {
   // Uninitialize here to let the browser process do it.
   ui::test::MaterialDesignControllerTestAPI::Uninitialize();
 
-  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  base::test::ScopedCommandLine scoped_command_line;

[Ilya Sherman, 2017/05/25 22:48:12]

I don't think it should be necessary to use a ScopedCommandLine here.  Does
anything go wrong if you modify the original command-line rather than a scoped
copy?

+  base::CommandLine* command_line = scoped_command_line.GetProcessCommandLine();
 
   // Override the child process connection timeout since tests can exceed that
   // when sharded.
@@ -241,11 +243,11 @@ void BrowserTestBase::SetUp() {
     base::FeatureList::GetInstance()->GetFeatureOverrides(&enabled_features,
                                                           &disabled_features);
   if (!enabled_features.empty())
-    command_line->AppendSwitchASCII(switches::kEnableFeatures,
-                                    enabled_features);
+    command_line->ReplaceSwitchASCIIForTesting(switches::kEnableFeatures,
+                                               enabled_features);

[Ilya Sherman, 2017/05/25 22:48:12]

I don't think that it's appropriate to just replace whatever switches were
already set by this point.  Instead, I think we should *remove* the switches
when they are read into the FeatureList in //base/test/test_suite; and we should
assert here that there are no --enable-features or --disable-features switches
present.  Otherwise, if some code tried to enable/disable features this way
between when the //base/test/test_suite code ran and when this code runs, it'll
silently fail, which is not good.

[chaopeng, 2017/05/26 00:57:41]

This is good to prevent teams using commandline to change features in SetUp.
Maybe I need a patch to fix that first.

   if (!disabled_features.empty())
-    command_line->AppendSwitchASCII(switches::kDisableFeatures,
-                                    disabled_features);
+    command_line->ReplaceSwitchASCIIForTesting(switches::kDisableFeatures,
+                                               disabled_features);
 
   // Need to wipe feature list clean, since BrowserMain calls
   // FeatureList::SetInstance, which expects no instance to exist.