aw: Set enable/disable feature once

Previous implementation appended a new --enable-feature or
--disable-feature switch for every hard coded feature. Command line argv
turned out to be append only, even though there is a remove switch
method.

Change to a scoped class and append each at most only one.

Change-Id: Ic0243d69bf23038c17a31a76f32ff2a88505028c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1526746
Commit-Queue: Bo <boliu@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#641830}
diff --git a/android_webview/BUILD.gn b/android_webview/BUILD.gn
index e7c16ac..0fad10a 100644
--- a/android_webview/BUILD.gn
+++ b/android_webview/BUILD.gn
@@ -585,8 +585,6 @@
     "browser/aw_web_contents_view_delegate.h",
     "browser/aw_web_ui_controller_factory.cc",
     "browser/aw_web_ui_controller_factory.h",
-    "browser/command_line_helper.cc",
-    "browser/command_line_helper.h",
     "browser/cookie_manager.cc",
     "browser/cookie_manager.h",
     "browser/find_helper.cc",
@@ -659,6 +657,8 @@
     "browser/safe_browsing/aw_safe_browsing_whitelist_manager.h",
     "browser/safe_browsing/aw_url_checker_delegate_impl.cc",
     "browser/safe_browsing/aw_url_checker_delegate_impl.h",
+    "browser/scoped_add_feature_flags.cc",
+    "browser/scoped_add_feature_flags.h",
     "browser/state_serializer.cc",
     "browser/state_serializer.h",
     "browser/tracing/aw_trace_event_args_whitelist.cc",
diff --git a/android_webview/browser/command_line_helper.cc b/android_webview/browser/command_line_helper.cc
deleted file mode 100644
index 81aea03..0000000
--- a/android_webview/browser/command_line_helper.cc
+++ /dev/null
@@ -1,65 +0,0 @@
-// Copyright 2016 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 "android_webview/browser/command_line_helper.h"
-
-#include <vector>
-
-#include "base/base_switches.h"
-#include "base/command_line.h"
-#include "base/feature_list.h"
-#include "base/stl_util.h"
-#include "base/strings/string_piece.h"
-#include "base/strings/string_util.h"
-
-using std::string;
-using std::vector;
-
-namespace {
-
-// Adds |feature_name| to the list of features in |feature_list_name|, but only
-// if the |feature_name| is absent from the list of features in both
-// |feature_list_name| and |other_feature_list_name|.
-void AddFeatureToList(base::CommandLine& command_line,
-                      const string& feature_name,
-                      const char* feature_list_name,
-                      const char* other_feature_list_name) {
-  const string features_list =
-      command_line.GetSwitchValueASCII(feature_list_name);
-  const string other_features_list =
-      command_line.GetSwitchValueASCII(other_feature_list_name);
-
-  if (features_list.empty() && other_features_list.empty()) {
-    command_line.AppendSwitchASCII(feature_list_name, feature_name);
-    return;
-  }
-
-  vector<base::StringPiece> features =
-      base::FeatureList::SplitFeatureListString(features_list);
-  vector<base::StringPiece> other_features =
-      base::FeatureList::SplitFeatureListString(other_features_list);
-
-  if (!base::ContainsValue(features, feature_name) &&
-      !base::ContainsValue(other_features, feature_name)) {
-    features.push_back(feature_name);
-    command_line.AppendSwitchASCII(feature_list_name,
-                                   base::JoinString(features, ","));
-  }
-}
-
-}  // namespace
-
-// static
-void CommandLineHelper::AddEnabledFeature(base::CommandLine& command_line,
-                                          const string& feature_name) {
-  AddFeatureToList(command_line, feature_name, switches::kEnableFeatures,
-                   switches::kDisableFeatures);
-}
-
-// static
-void CommandLineHelper::AddDisabledFeature(base::CommandLine& command_line,
-                                           const string& feature_name) {
-  AddFeatureToList(command_line, feature_name, switches::kDisableFeatures,
-                   switches::kEnableFeatures);
-}
diff --git a/android_webview/browser/command_line_helper.h b/android_webview/browser/command_line_helper.h
deleted file mode 100644
index 9ad4326..0000000
--- a/android_webview/browser/command_line_helper.h
+++ /dev/null
@@ -1,32 +0,0 @@
-// Copyright 2016 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.
-
-#ifndef ANDROID_WEBVIEW_BROWSER_COMMAND_LINE_HELPER_H_
-#define ANDROID_WEBVIEW_BROWSER_COMMAND_LINE_HELPER_H_
-
-#include <string>
-
-#include "base/macros.h"
-
-namespace base {
-class CommandLine;
-}  // namespace base
-
-class CommandLineHelper {
- public:
-  // Add a feature to the --enabled-features list, but only if it's not
-  // already enabled or disabled.
-  static void AddEnabledFeature(base::CommandLine& command_line,
-                                const std::string& feature_name);
-
-  // Add a feature to the --disabled-features list, but only if it's not already
-  // enabled or disabled.
-  static void AddDisabledFeature(base::CommandLine& command_line,
-                                 const std::string& feature_name);
-
- private:
-  DISALLOW_IMPLICIT_CONSTRUCTORS(CommandLineHelper);
-};
-
-#endif  // ANDROID_WEBVIEW_BROWSER_COMMAND_LINE_HELPER_H_
diff --git a/android_webview/browser/command_line_helper_unittest.cc b/android_webview/browser/command_line_helper_unittest.cc
deleted file mode 100644
index c2710a9..0000000
--- a/android_webview/browser/command_line_helper_unittest.cc
+++ /dev/null
@@ -1,134 +0,0 @@
-// Copyright 2016 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 "android_webview/browser/command_line_helper.h"
-
-#include "base/base_switches.h"
-#include "base/command_line.h"
-#include "base/feature_list.h"
-#include "base/files/file_path.h"
-#include "base/stl_util.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-using testing::Test;
-using base::CommandLine;
-
-const base::Feature kSomeSpecialFeature{"SomeSpecialFeature",
-                                        base::FEATURE_DISABLED_BY_DEFAULT};
-
-class CommandLineHelperTest : public Test {
- public:
-  CommandLineHelperTest() {}
-
-  void EnableFeatureAndVerify(CommandLine& command_line,
-                              const std::string& enabled_expected,
-                              const std::string& disabled_expected) {
-    CommandLineHelper::AddEnabledFeature(command_line,
-                                         kSomeSpecialFeature.name);
-    Verify(command_line, enabled_expected, disabled_expected);
-  }
-
-  void DisableFeatureAndVerify(CommandLine& command_line,
-                               const std::string& enabled_expected,
-                               const std::string& disabled_expected) {
-    CommandLineHelper::AddDisabledFeature(command_line,
-                                          kSomeSpecialFeature.name);
-    Verify(command_line, enabled_expected, disabled_expected);
-  }
-
-  void Verify(const CommandLine& command_line,
-              const std::string& enabled_expected,
-              const std::string& disabled_expected) {
-    EXPECT_EQ(enabled_expected,
-              command_line.GetSwitchValueASCII(switches::kEnableFeatures));
-    EXPECT_EQ(disabled_expected,
-              command_line.GetSwitchValueASCII(switches::kDisableFeatures));
-  }
-};
-
-TEST_F(CommandLineHelperTest, EnableForEmptyCommandLine) {
-  CommandLine command_line(CommandLine::NO_PROGRAM);
-  EnableFeatureAndVerify(command_line, "SomeSpecialFeature", "");
-}
-
-TEST_F(CommandLineHelperTest, EnableForNoEnabledFeatures) {
-  const CommandLine::CharType* argv[] = {FILE_PATH_LITERAL("program")};
-  CommandLine command_line(base::size(argv), argv);
-  EnableFeatureAndVerify(command_line, "SomeSpecialFeature", "");
-}
-
-TEST_F(CommandLineHelperTest, EnableForEnabledTestFeature) {
-  const CommandLine::CharType* argv[] = {
-      FILE_PATH_LITERAL("program"),
-      FILE_PATH_LITERAL("--enable-features=TestFeature")};
-  CommandLine command_line(base::size(argv), argv);
-  EnableFeatureAndVerify(command_line, "TestFeature,SomeSpecialFeature", "");
-}
-
-TEST_F(CommandLineHelperTest, EnableForEnabledSomeSpecialFeature) {
-  const CommandLine::CharType* argv[] = {
-      FILE_PATH_LITERAL("program"),
-      FILE_PATH_LITERAL("--enable-features=SomeSpecialFeature,TestFeature")};
-  CommandLine command_line(base::size(argv), argv);
-  EnableFeatureAndVerify(command_line, "SomeSpecialFeature,TestFeature", "");
-}
-
-TEST_F(CommandLineHelperTest, EnableForDisabledSomeSpecialFeature) {
-  const CommandLine::CharType* argv[] = {
-      FILE_PATH_LITERAL("program"),
-      FILE_PATH_LITERAL("--disable-features=SomeSpecialFeature")};
-  CommandLine command_line(base::size(argv), argv);
-  EnableFeatureAndVerify(command_line, "", "SomeSpecialFeature");
-}
-
-TEST_F(CommandLineHelperTest, EnableForDisabledTestFeature) {
-  const CommandLine::CharType* argv[] = {
-      FILE_PATH_LITERAL("program"),
-      FILE_PATH_LITERAL("--disable-features=TestFeature")};
-  CommandLine command_line(base::size(argv), argv);
-  EnableFeatureAndVerify(command_line, "SomeSpecialFeature", "TestFeature");
-}
-
-TEST_F(CommandLineHelperTest, DisableForEmptyCommandLine) {
-  CommandLine command_line(CommandLine::NO_PROGRAM);
-  DisableFeatureAndVerify(command_line, "", "SomeSpecialFeature");
-}
-
-TEST_F(CommandLineHelperTest, DisableForNoDisabledFeatures) {
-  const CommandLine::CharType* argv[] = {FILE_PATH_LITERAL("program")};
-  CommandLine command_line(base::size(argv), argv);
-  DisableFeatureAndVerify(command_line, "", "SomeSpecialFeature");
-}
-
-TEST_F(CommandLineHelperTest, DisableForDisabledTestFeature) {
-  const CommandLine::CharType* argv[] = {
-      FILE_PATH_LITERAL("program"),
-      FILE_PATH_LITERAL("--disable-features=TestFeature")};
-  CommandLine command_line(base::size(argv), argv);
-  DisableFeatureAndVerify(command_line, "", "TestFeature,SomeSpecialFeature");
-}
-
-TEST_F(CommandLineHelperTest, DisableForDisabledSomeSpecialFeature) {
-  const CommandLine::CharType* argv[] = {
-      FILE_PATH_LITERAL("program"),
-      FILE_PATH_LITERAL("--disable-features=SomeSpecialFeature,TestFeature")};
-  CommandLine command_line(base::size(argv), argv);
-  DisableFeatureAndVerify(command_line, "", "SomeSpecialFeature,TestFeature");
-}
-
-TEST_F(CommandLineHelperTest, DisableForEnabledSomeSpecialFeature) {
-  const CommandLine::CharType* argv[] = {
-      FILE_PATH_LITERAL("program"),
-      FILE_PATH_LITERAL("--enable-features=SomeSpecialFeature")};
-  CommandLine command_line(base::size(argv), argv);
-  DisableFeatureAndVerify(command_line, "SomeSpecialFeature", "");
-}
-
-TEST_F(CommandLineHelperTest, DisableForEnabledTestFeature) {
-  const CommandLine::CharType* argv[] = {
-      FILE_PATH_LITERAL("program"),
-      FILE_PATH_LITERAL("--enable-features=TestFeature")};
-  CommandLine command_line(base::size(argv), argv);
-  DisableFeatureAndVerify(command_line, "TestFeature", "SomeSpecialFeature");
-}
diff --git a/android_webview/browser/scoped_add_feature_flags.cc b/android_webview/browser/scoped_add_feature_flags.cc
new file mode 100644
index 0000000..03f80fd
--- /dev/null
+++ b/android_webview/browser/scoped_add_feature_flags.cc
@@ -0,0 +1,50 @@
+// Copyright 2019 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 "android_webview/browser/scoped_add_feature_flags.h"
+
+#include "base/base_switches.h"
+#include "base/command_line.h"
+#include "base/stl_util.h"
+#include "base/strings/string_util.h"
+
+namespace android_webview {
+
+ScopedAddFeatureFlags::ScopedAddFeatureFlags(base::CommandLine* cl)
+    : cl_(cl),
+      enabled_features_(base::FeatureList::SplitFeatureListString(
+          cl->GetSwitchValueASCII(switches::kEnableFeatures))),
+      disabled_features_(base::FeatureList::SplitFeatureListString(
+          cl->GetSwitchValueASCII(switches::kDisableFeatures))) {}
+
+ScopedAddFeatureFlags::~ScopedAddFeatureFlags() {
+  cl_->AppendSwitchASCII(switches::kEnableFeatures,
+                         base::JoinString(enabled_features_, ","));
+  cl_->AppendSwitchASCII(switches::kDisableFeatures,
+                         base::JoinString(disabled_features_, ","));
+}
+
+void ScopedAddFeatureFlags::EnableIfNotSet(const base::Feature& feature) {
+  AddFeatureIfNotSet(feature, true /* enable */);
+}
+
+void ScopedAddFeatureFlags::DisableIfNotSet(const base::Feature& feature) {
+  AddFeatureIfNotSet(feature, false /* enable */);
+}
+
+void ScopedAddFeatureFlags::AddFeatureIfNotSet(const base::Feature& feature,
+                                               bool enable) {
+  const char* feature_name = feature.name;
+  if (base::ContainsValue(enabled_features_, feature_name) ||
+      base::ContainsValue(disabled_features_, feature_name)) {
+    return;
+  }
+  if (enable) {
+    enabled_features_.emplace_back(feature_name);
+  } else {
+    disabled_features_.emplace_back(feature_name);
+  }
+}
+
+}  // namespace android_webview
diff --git a/android_webview/browser/scoped_add_feature_flags.h b/android_webview/browser/scoped_add_feature_flags.h
new file mode 100644
index 0000000..0b990ec
--- /dev/null
+++ b/android_webview/browser/scoped_add_feature_flags.h
@@ -0,0 +1,40 @@
+// Copyright 2019 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.
+
+#ifndef ANDROID_WEBVIEW_BROWSER_SCOPED_ADD_FEATURE_FLAGS_H_
+#define ANDROID_WEBVIEW_BROWSER_SCOPED_ADD_FEATURE_FLAGS_H_
+
+#include <vector>
+
+#include "base/feature_list.h"
+#include "base/strings/string_piece.h"
+
+namespace base {
+class CommandLine;
+}
+
+namespace android_webview {
+
+class ScopedAddFeatureFlags {
+ public:
+  explicit ScopedAddFeatureFlags(base::CommandLine* cl);
+  ~ScopedAddFeatureFlags();
+
+  // Any existing (user set) enable/disable takes precedence.
+  void EnableIfNotSet(const base::Feature& feature);
+  void DisableIfNotSet(const base::Feature& feature);
+
+ private:
+  void AddFeatureIfNotSet(const base::Feature& feature, bool enable);
+
+  base::CommandLine* const cl_;
+  std::vector<base::StringPiece> enabled_features_;
+  std::vector<base::StringPiece> disabled_features_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedAddFeatureFlags);
+};
+
+}  // namespace android_webview
+
+#endif  // ANDROID_WEBVIEW_BROWSER_SCOPED_ADD_FEATURE_FLAGS_H_
diff --git a/android_webview/lib/aw_main_delegate.cc b/android_webview/lib/aw_main_delegate.cc
index 48ce7bc..f04f892 100644
--- a/android_webview/lib/aw_main_delegate.cc
+++ b/android_webview/lib/aw_main_delegate.cc
@@ -8,9 +8,9 @@
 
 #include "android_webview/browser/aw_content_browser_client.h"
 #include "android_webview/browser/aw_media_url_interceptor.h"
-#include "android_webview/browser/command_line_helper.h"
 #include "android_webview/browser/gfx/browser_view_renderer.h"
 #include "android_webview/browser/gfx/deferred_gpu_command_service.h"
+#include "android_webview/browser/scoped_add_feature_flags.h"
 #include "android_webview/browser/tracing/aw_trace_event_args_whitelist.h"
 #include "android_webview/common/aw_descriptors.h"
 #include "android_webview/common/aw_paths.h"
@@ -163,46 +163,43 @@
     cl->AppendSwitch(switches::kInProcessGPU);
   }
 
+  {
+    ScopedAddFeatureFlags features(cl);
+
 #if BUILDFLAG(ENABLE_SPELLCHECK)
-  CommandLineHelper::AddEnabledFeature(
-      *cl, spellcheck::kAndroidSpellCheckerNonLowEnd.name);
+    features.EnableIfNotSet(spellcheck::kAndroidSpellCheckerNonLowEnd);
 #endif  // ENABLE_SPELLCHECK
 
-  CommandLineHelper::AddDisabledFeature(*cl, features::kWebPayments.name);
+    features.EnableIfNotSet(
+        autofill::features::kAutofillSkipComparingInferredLabels);
 
-  // WebView does not and should not support WebAuthN.
-  CommandLineHelper::AddDisabledFeature(*cl, features::kWebAuth.name);
+    features.DisableIfNotSet(features::kWebPayments);
 
-  // WebView isn't compatible with OOP-D.
-  CommandLineHelper::AddDisabledFeature(*cl,
-                                        features::kVizDisplayCompositor.name);
+    // WebView does not and should not support WebAuthN.
+    features.DisableIfNotSet(features::kWebAuth);
 
-  // WebView does not support AndroidOverlay yet for video overlays.
-  CommandLineHelper::AddDisabledFeature(*cl, media::kUseAndroidOverlay.name);
+    // WebView isn't compatible with OOP-D.
+    features.DisableIfNotSet(features::kVizDisplayCompositor);
 
-  // WebView doesn't support embedding CompositorFrameSinks which is needed for
-  // UseSurfaceLayerForVideo[PIP] feature. https://crbug.com/853832
-  CommandLineHelper::AddDisabledFeature(*cl,
-                                        media::kUseSurfaceLayerForVideo.name);
-  CommandLineHelper::AddDisabledFeature(
-      *cl, media::kUseSurfaceLayerForVideoPIP.name);
+    // WebView does not support AndroidOverlay yet for video overlays.
+    features.DisableIfNotSet(media::kUseAndroidOverlay);
 
-  // WebView does not support EME persistent license yet, because it's not
-  // clear on how user can remove persistent media licenses from UI.
-  CommandLineHelper::AddDisabledFeature(*cl,
-                                        media::kMediaDrmPersistentLicense.name);
+    // WebView doesn't support embedding CompositorFrameSinks which is needed
+    // for UseSurfaceLayerForVideo[PIP] feature. https://crbug.com/853832
+    features.DisableIfNotSet(media::kUseSurfaceLayerForVideo);
+    features.DisableIfNotSet(media::kUseSurfaceLayerForVideoPIP);
 
-  CommandLineHelper::AddEnabledFeature(
-      *cl, autofill::features::kAutofillSkipComparingInferredLabels.name);
+    // WebView does not support EME persistent license yet, because it's not
+    // clear on how user can remove persistent media licenses from UI.
+    features.DisableIfNotSet(media::kMediaDrmPersistentLicense);
 
-  CommandLineHelper::AddDisabledFeature(
-      *cl, autofill::features::kAutofillRestrictUnownedFieldsToFormlessCheckout
-               .name);
+    features.DisableIfNotSet(
+        autofill::features::kAutofillRestrictUnownedFieldsToFormlessCheckout);
 
-  CommandLineHelper::AddDisabledFeature(*cl, features::kBackgroundFetch.name);
+    features.DisableIfNotSet(features::kBackgroundFetch);
 
-  CommandLineHelper::AddDisabledFeature(*cl,
-                                        features::kAndroidSurfaceControl.name);
+    features.DisableIfNotSet(features::kAndroidSurfaceControl);
+  }
 
   android_webview::RegisterPathProvider();
 
diff --git a/android_webview/test/BUILD.gn b/android_webview/test/BUILD.gn
index dcc850f..56c1a68 100644
--- a/android_webview/test/BUILD.gn
+++ b/android_webview/test/BUILD.gn
@@ -337,7 +337,6 @@
     "../browser/aw_media_url_interceptor_unittest.cc",
     "../browser/aw_permission_manager_unittest.cc",
     "../browser/aw_static_cookie_policy_unittest.cc",
-    "../browser/command_line_helper_unittest.cc",
     "../browser/gfx/browser_view_renderer_unittest.cc",
     "../browser/gfx/test/fake_window.cc",
     "../browser/gfx/test/fake_window.h",