Support Using ScopedFeatureList in BrowserTest

content/public/test/browser_test_base.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/public/test/browser_test_base.h"
 
 #include <stddef.h>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/debug/stack_trace.h"
 #include "base/feature_list.h"
 #include "base/i18n/icu_util.h"
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #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"
 #include "build/build_config.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/tracing/tracing_controller_impl.h"
 #include "content/public/app/content_main.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/main_function_params.h"
@@ skipping 114 matching lines @@
                            "does not run and reports a false positive result.";
 }
 
 void BrowserTestBase::SetUp() {
   set_up_called_ = true;
   // ContentTestSuiteBase might have already initialized
   // MaterialDesignController in browser_tests suite.
   // 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.
   command_line->AppendSwitchASCII(
       switches::kIPCConnectionTimeout,
       base::Int64ToString(TestTimeouts::action_max_timeout().InSeconds()));
 
   // The tests assume that file:// URIs can freely access other file:// URIs.
   command_line->AppendSwitch(switches::kAllowFileAccessFromFiles);
 
@@ skipping 69 matching lines @@
   SetUpInProcessBrowserTestFixture();
 
   // At this point, copy features to the command line, since BrowserMain will
   // wipe out the current feature list.
   std::string enabled_features;
   std::string disabled_features;
   if (base::FeatureList::GetInstance())
     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.
   base::FeatureList::ClearInstanceForTesting();
 
   base::Closure* ui_task =
       new base::Closure(
           base::Bind(&BrowserTestBase::ProxyRunTestOnMainThreadLoop,
                      base::Unretained(this)));
 
@@ skipping 149 matching lines @@
   if (mojo_rules.empty())
     return;
 
   mojom::NetworkServiceTestPtr network_service_test;
   ServiceManagerConnection::GetForProcess()->GetConnector()->BindInterface(
       mojom::kNetworkServiceName, &network_service_test);
   network_service_test->AddRules(std::move(mojo_rules));
 }
 
 }  // namespace content