[Extensions Toolbar] Add a finch config for the redesign

chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc

 // Copyright (c) 2012 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 "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_action_test_util.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/extensions/test_extension_dir.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "content/public/test/browser_test_utils.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
+#include "extensions/browser/test_extension_registry_observer.h"
 #include "extensions/test/extension_test_message_listener.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
 namespace extensions {
 namespace {
 
 const char kDeclarativeContentManifest[] =
     "{\n"
     "  \"name\": \"Declarative Content apitest\",\n"
     "  \"version\": \"0.1\",\n"
@@ skipping 602 matching lines @@
             ExecuteScriptInBackgroundPage(
                 extension->id(),
                 "onPageChanged.removeRules(undefined, function() {\n"
                 "  window.domAutomationController.send('removed');\n"
                 "});\n"));
   EXPECT_FALSE(page_action->GetIsVisible(tab_id));
 }
 
 IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest,
                        ShowPageActionWithoutPageAction) {
+  // Load an extension without a page action.
   std::string manifest_without_page_action = kDeclarativeContentManifest;
   base::ReplaceSubstringsAfterOffset(
       &manifest_without_page_action, 0, "\"page_action\": {},", "");
   ASSERT_NE(kDeclarativeContentManifest, manifest_without_page_action);
   ext_dir_.WriteManifest(manifest_without_page_action);
   ext_dir_.WriteFile(FILE_PATH_LITERAL("background.js"), kBackgroundHelpers);
-  const Extension* extension = LoadExtension(ext_dir_.unpacked_path());
-  ASSERT_TRUE(extension);
 
-  EXPECT_THAT(ExecuteScriptInBackgroundPage(
-                  extension->id(),
-                  "setRules([{\n"
-                  "  conditions: [new PageStateMatcher({\n"
-                  "                   pageUrl: {hostPrefix: \"test\"}})],\n"
-                  "  actions: [new ShowPageAction()]\n"
-                  "}], 'test_rule');\n"),
-              testing::HasSubstr("without a page action"));
+  std::string extension_id;
 
-  content::WebContents* const tab =
-      browser()->tab_strip_model()->GetWebContentsAt(0);
-  NavigateInRenderer(tab, GURL("http://test/"));
+  {
+    // Without the extension action redesign, the extension should get an error.
+    FeatureSwitch::ScopedOverride override_toolbar_redesign(

[Devlin, 2015/09/21 23:28:15]

This first block is identical to the one of the left, except for the feature
switch here.

The second block is also identical, except for the feature switch and different
"expects".

[Finnur, 2015/09/22 14:18:32]

I probably would have created a helper function instead, but since this clause
goes away when the switch goes away, this is probably fine for now.

[Devlin, 2015/09/23 00:28:35]

I was thinking about it, and there were enough differences (the switch,
reloading vs loading the extension, the test asserts, etc) that it really
wouldn't have saved much, and, like you said, it'll go away once the switch is
default.

But I can clean up one of the really ugly repeated bits, which is the hard-coded
script.  Pulled that out into a variable to make it a little nicer.

+        FeatureSwitch::extension_action_redesign(), false);
+    const Extension* extension = LoadExtension(ext_dir_.unpacked_path());
+    ASSERT_TRUE(extension);
+    extension_id = extension->id();
 
-  EXPECT_EQ(NULL,
-            ExtensionActionManager::Get(browser()->profile())->
-                GetPageAction(*extension));
-  EXPECT_EQ(0u, extension_action_test_util::GetVisiblePageActionCount(tab));
+    EXPECT_THAT(ExecuteScriptInBackgroundPage(
+                    extension->id(),
+                    "setRules([{\n"
+                    "  conditions: [new PageStateMatcher({\n"
+                    "                   pageUrl: {hostPrefix: \"test\"}})],\n"
+                    "  actions: [new ShowPageAction()]\n"
+                    "}], 'test_rule');\n"),
+                testing::HasSubstr("without a page action"));
+
+    content::WebContents* const tab =
+        browser()->tab_strip_model()->GetWebContentsAt(0);
+    NavigateInRenderer(tab, GURL("http://test/"));
+
+    // There should be no page action.
+    EXPECT_EQ(nullptr, ExtensionActionManager::Get(browser()->profile())
+                           ->GetPageAction(*extension));
+    EXPECT_EQ(0u, extension_action_test_util::GetVisiblePageActionCount(tab));
+  }
+  {
+    // With the extension action redesign, it should work normally.
+    FeatureSwitch::ScopedOverride override_toolbar_redesign(
+        FeatureSwitch::extension_action_redesign(), true);
+    ReloadExtension(extension_id);
+    const Extension* extension =
+        ExtensionRegistry::Get(profile())->enabled_extensions().GetByID(
+            extension_id);
+    ASSERT_TRUE(extension);
+
+    EXPECT_THAT(ExecuteScriptInBackgroundPage(
+                    extension->id(),
+                    "setRules([{\n"
+                    "  conditions: [new PageStateMatcher({\n"
+                    "                   pageUrl: {hostPrefix: \"test\"}})],\n"
+                    "  actions: [new ShowPageAction()]\n"
+                    "}], 'test_rule');\n"),
+                testing::Not(testing::HasSubstr("without a page action")));
+
+    content::WebContents* const tab =
+        browser()->tab_strip_model()->GetWebContentsAt(0);
+    NavigateInRenderer(tab, GURL("http://test/"));
+
+    // There should be a page action.
+    EXPECT_TRUE(ExtensionActionManager::Get(browser()->profile())
+                    ->GetPageAction(*extension));
+    EXPECT_EQ(1u, extension_action_test_util::GetVisiblePageActionCount(tab));
+  }
 }
 
 IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest,
                        CanonicalizesPageStateMatcherCss) {
   ext_dir_.WriteManifest(kDeclarativeContentManifest);
   ext_dir_.WriteFile(
       FILE_PATH_LITERAL("background.js"),
       "var PageStateMatcher = chrome.declarativeContent.PageStateMatcher;\n"
       "function Return(obj) {\n"
       "  window.domAutomationController.send('' + obj);\n"
@@ skipping 182 matching lines @@
 // action interfaces, add a test that checks that a navigation always evaluates
 // consistent URL state for all conditions. i.e.: if condition1 evaluates to
 // false on url0 and true on url1, and condition2 evaluates to true on url0 and
 // false on url1, navigate from url0 to url1 and validate that no action is
 // triggered. Do the same when navigating back to url0. This kind of test is
 // unfortunately not feasible with the current implementation and the existing
 // supported conditions and actions.
 
 }  // namespace
 }  // namespace extensions