[Extensions Toolbar] Add a finch config for the redesign

Add an entry in field trials for the extension action redesign, and
fix any tests that fail with it as a default.

BUG=533101
TBR=avi@chromium.org for micro cocoa change

Review URL: https://codereview.chromium.org/1363463002

Cr-Commit-Position: refs/heads/master@{#350296}

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

@@ -21,6 +21,7 @@
 #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"
 
@@ -643,32 +644,65 @@ IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest,
 
 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/"));
+  std::string script =
+      "setRules([{\n"
+      "  conditions: [new PageStateMatcher({\n"
+      "                   pageUrl: {hostPrefix: \"test\"}})],\n"
+      "  actions: [new ShowPageAction()]\n"
+      "}], 'test_rule');\n";
 
-  EXPECT_EQ(NULL,
-            ExtensionActionManager::Get(browser()->profile())->
-                GetPageAction(*extension));
-  EXPECT_EQ(0u, extension_action_test_util::GetVisiblePageActionCount(tab));
+  {
+    // Without the extension action redesign, the extension should get an error.
+    FeatureSwitch::ScopedOverride override_toolbar_redesign(
+        FeatureSwitch::extension_action_redesign(), false);
+    const Extension* extension = LoadExtension(ext_dir_.unpacked_path());
+    ASSERT_TRUE(extension);
+    extension_id = extension->id();
+
+    EXPECT_THAT(ExecuteScriptInBackgroundPage(extension->id(), script),
+                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(), script),
+                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,

chrome/browser/extensions/extension_action.cc

@@ -256,15 +256,20 @@ gfx::Image ExtensionAction::GetDefaultIconImage() const {
   if (default_icon_image_)
     return default_icon_image_->image();
 
-  // If the extension action redesign is enabled, we use a special placeholder
-  // icon (with the first letter of the extension name) rather than the default
-  // (puzzle piece).
-  if (extensions::FeatureSwitch::extension_action_redesign()->IsEnabled()) {
-    return extensions::ExtensionIconPlaceholder::CreateImage(
-        extension_misc::EXTENSION_ICON_ACTION, extension_name_);
+  if (placeholder_icon_image_.IsEmpty()) {
+    // If the extension action redesign is enabled, we use a special placeholder
+    // icon (with the first letter of the extension name) rather than the
+    // default (puzzle piece).
+    if (extensions::FeatureSwitch::extension_action_redesign()->IsEnabled()) {
+      placeholder_icon_image_ =
+          extensions::ExtensionIconPlaceholder::CreateImage(
+              extension_misc::EXTENSION_ICON_ACTION, extension_name_);
+    } else {
+      placeholder_icon_image_ = GetDefaultIcon();
+    }
   }
 
-  return GetDefaultIcon();
+  return placeholder_icon_image_;
 }
 
 bool ExtensionAction::HasPopupUrl(int tab_id) const {

chrome/browser/extensions/extension_action.h

@@ -13,6 +13,7 @@
 #include "base/stl_util.h"
 #include "chrome/common/extensions/api/extension_action/action_info.h"
 #include "third_party/skia/include/core/SkColor.h"
+#include "ui/gfx/image/image.h"
 
 class GURL;
 
@@ -287,6 +288,11 @@ class ExtensionAction {
   // Lazily initialized via LoadDefaultIconImage().
   scoped_ptr<extensions::IconImage> default_icon_image_;
 
+  // The lazily-initialized image for a placeholder icon, in the event that the
+  // extension doesn't have its own icon. (Mutable to allow lazy init in
+  // GetDefaultIconImage().)
+  mutable gfx::Image placeholder_icon_image_;
+
   // The id for the ExtensionAction, for example: "RssPageAction". This is
   // needed for compat with an older version of the page actions API.
   std::string id_;

chrome/browser/extensions/lazy_background_page_apitest.cc

@@ -9,6 +9,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/extensions/browser_action_test_util.h"
+#include "chrome/browser/extensions/extension_action_test_util.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/lazy_background_page_test_util.h"
 #include "chrome/browser/profiles/profile.h"
@@ -41,8 +42,8 @@
 
 using bookmarks::BookmarkModel;
 using bookmarks::BookmarkNode;
-using extensions::Extension;
-using extensions::ResultCatcher;
+
+namespace extensions {
 
 namespace {
 
@@ -52,12 +53,11 @@ namespace {
 // incognito involves reloading the extension - and the background pages may
 // have already loaded once before then. So we wait until the extension is
 // unloaded before listening to the background page notifications.
-class LoadedIncognitoObserver : public extensions::ExtensionRegistryObserver {
+class LoadedIncognitoObserver : public ExtensionRegistryObserver {
  public:
   explicit LoadedIncognitoObserver(Profile* profile)
       : profile_(profile), extension_registry_observer_(this) {
-    extension_registry_observer_.Add(
-        extensions::ExtensionRegistry::Get(profile_));
+    extension_registry_observer_.Add(ExtensionRegistry::Get(profile_));
   }
 
   void Wait() {
@@ -67,18 +67,16 @@ class LoadedIncognitoObserver : public extensions::ExtensionRegistryObserver {
   }
 
  private:
-  void OnExtensionUnloaded(
-      content::BrowserContext* browser_context,
-      const Extension* extension,
-      extensions::UnloadedExtensionInfo::Reason reason) override {
+  void OnExtensionUnloaded(content::BrowserContext* browser_context,
+                           const Extension* extension,
+                           UnloadedExtensionInfo::Reason reason) override {
     original_complete_.reset(new LazyBackgroundObserver(profile_));
     incognito_complete_.reset(
         new LazyBackgroundObserver(profile_->GetOffTheRecordProfile()));
   }
 
   Profile* profile_;
-  ScopedObserver<extensions::ExtensionRegistry,
-                 extensions::ExtensionRegistryObserver>
+  ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>
       extension_registry_observer_;
   scoped_ptr<LazyBackgroundObserver> original_complete_;
   scoped_ptr<LazyBackgroundObserver> incognito_complete_;
@@ -94,8 +92,8 @@ class LazyBackgroundPageApiTest : public ExtensionApiTest {
   void SetUpInProcessBrowserTestFixture() override {
     ExtensionApiTest::SetUpInProcessBrowserTestFixture();
     // Set shorter delays to prevent test timeouts.
-    extensions::ProcessManager::SetEventPageIdleTimeForTesting(1);
-    extensions::ProcessManager::SetEventPageSuspendingTimeForTesting(1);
+    ProcessManager::SetEventPageIdleTimeForTesting(1);
+    ProcessManager::SetEventPageSuspendingTimeForTesting(1);
   }
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
@@ -121,8 +119,7 @@ class LazyBackgroundPageApiTest : public ExtensionApiTest {
   // Returns true if the lazy background page for the extension with
   // |extension_id| is still running.
   bool IsBackgroundPageAlive(const std::string& extension_id) {
-    extensions::ProcessManager* pm =
-        extensions::ProcessManager::Get(browser()->profile());
+    ProcessManager* pm = ProcessManager::Get(browser()->profile());
     return pm->GetBackgroundHostForExtension(extension_id);
   }
 
@@ -191,9 +188,9 @@ IN_PROC_BROWSER_TEST_F(LazyBackgroundPageApiTest, BroadcastEvent) {
 
   // Page action is shown.
   WaitForPageActionVisibilityChangeTo(num_page_actions + 1);
-  EXPECT_EQ(num_page_actions + 1,
-            browser()->window()->GetLocationBar()->
-                GetLocationBarForTesting()->PageActionVisibleCount());
+  EXPECT_EQ(static_cast<size_t>(num_page_actions + 1),
+            extension_action_test_util::GetVisiblePageActionCount(
+                browser()->tab_strip_model()->GetActiveWebContents()));
 }
 
 IN_PROC_BROWSER_TEST_F(LazyBackgroundPageApiTest, Filters) {
@@ -239,8 +236,7 @@ IN_PROC_BROWSER_TEST_F(LazyBackgroundPageApiTest, WaitForDialog) {
   EXPECT_TRUE(IsBackgroundPageAlive(extension->id()));
 
   // Close the dialog. The keep alive count is decremented.
-  extensions::ProcessManager* pm =
-      extensions::ProcessManager::Get(browser()->profile());
+  ProcessManager* pm = ProcessManager::Get(browser()->profile());
   int previous_keep_alive_count = pm->GetLazyKeepaliveCount(extension);
   dialog->CloseModalDialog();
   EXPECT_EQ(previous_keep_alive_count - 1,
@@ -295,9 +291,8 @@ IN_PROC_BROWSER_TEST_F(LazyBackgroundPageApiTest, WaitForRequest) {
   EXPECT_TRUE(catcher.GetNextResult()) << catcher.message();
 
   // Lazy Background Page still exists, because the extension started a request.
-  extensions::ProcessManager* pm =
-      extensions::ProcessManager::Get(browser()->profile());
-  extensions::ExtensionHost* host =
+  ProcessManager* pm = ProcessManager::Get(browser()->profile());
+  ExtensionHost* host =
       pm->GetBackgroundHostForExtension(last_loaded_extension_id());
   ASSERT_TRUE(host);
 
@@ -399,10 +394,8 @@ IN_PROC_BROWSER_TEST_F(LazyBackgroundPageApiTest, DISABLED_IncognitoSplitMode) {
   }
 
   // Lazy Background Page doesn't exist yet.
-  extensions::ProcessManager* pm =
-      extensions::ProcessManager::Get(browser()->profile());
-  extensions::ProcessManager* pmi =
-      extensions::ProcessManager::Get(incognito_browser->profile());
+  ProcessManager* pm = ProcessManager::Get(browser()->profile());
+  ProcessManager* pmi = ProcessManager::Get(incognito_browser->profile());
   EXPECT_FALSE(pm->GetBackgroundHostForExtension(last_loaded_extension_id()));
   EXPECT_FALSE(pmi->GetBackgroundHostForExtension(last_loaded_extension_id()));
 
@@ -488,8 +481,7 @@ IN_PROC_BROWSER_TEST_F(LazyBackgroundPageApiTest, ImpulseAddsCount) {
   ASSERT_TRUE(extension);
 
   // Lazy Background Page doesn't exist yet.
-  extensions::ProcessManager* pm =
-      extensions::ProcessManager::Get(browser()->profile());
+  ProcessManager* pm = ProcessManager::Get(browser()->profile());
   EXPECT_FALSE(pm->GetBackgroundHostForExtension(last_loaded_extension_id()));
   EXPECT_EQ(1, browser()->tab_strip_model()->count());
 
@@ -535,8 +527,7 @@ IN_PROC_BROWSER_TEST_F(LazyBackgroundPageApiTest, EventDispatchToTab) {
   ResultCatcher catcher;
   catcher.RestrictToBrowserContext(browser()->profile());
 
-  const extensions::Extension* extension =
-      LoadExtensionAndWait("event_dispatch_to_tab");
+  const Extension* extension = LoadExtensionAndWait("event_dispatch_to_tab");
 
   ExtensionTestMessageListener page_ready("ready", true);
   GURL page_url = extension->GetResourceURL("page.html");
@@ -630,3 +621,5 @@ IN_PROC_BROWSER_TEST_F(LazyBackgroundPageApiTest, OnSuspendUseStorageApi) {
 
 // TODO: background page with timer.
 // TODO: background page that interacts with popup.
+
+}  // namespace extensions

chrome/browser/extensions/location_bar_controller.cc

@@ -30,8 +30,7 @@ LocationBarController::LocationBarController(
       should_show_page_actions_(
           !FeatureSwitch::extension_action_redesign()->IsEnabled()),
       extension_registry_observer_(this) {
-  if (should_show_page_actions_)
-    extension_registry_observer_.Add(ExtensionRegistry::Get(browser_context_));
+  extension_registry_observer_.Add(ExtensionRegistry::Get(browser_context_));
 }
 
 LocationBarController::~LocationBarController() {
@@ -93,7 +92,7 @@ std::vector<ExtensionAction*> LocationBarController::GetCurrentActions() {
 void LocationBarController::OnExtensionLoaded(
     content::BrowserContext* browser_context,
     const Extension* extension) {
-  if (action_manager_->GetPageAction(*extension)) {
+  if (should_show_page_actions_ && action_manager_->GetPageAction(*extension)) {
     ExtensionActionAPI::Get(browser_context)->
         NotifyPageActionsChanged(web_contents_);
   }
@@ -122,7 +121,7 @@ void LocationBarController::OnExtensionUnloaded(
     content::BrowserContext* browser_context,
     const Extension* extension,
     UnloadedExtensionInfo::Reason reason) {
-  if (action_manager_->GetPageAction(*extension)) {
+  if (should_show_page_actions_ && action_manager_->GetPageAction(*extension)) {
     ExtensionActionAPI::Get(browser_context)->
         NotifyPageActionsChanged(web_contents_);
   }

chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm

@@ -234,10 +234,12 @@ bool ExtensionInstalledBubbleBridge::MaybeShowNow() {
         NSMaxY(bounds) - extension_installed_bubble::kAppsBubbleArrowOffset);
     arrowPoint = [button convertPoint:anchor toView:nil];
   } else if (type_ == extension_installed_bubble::kBrowserAction ||
-             extensions::FeatureSwitch::extension_action_redesign()->
-                 IsEnabled()) {
+             (extensions::FeatureSwitch::extension_action_redesign()->
+                  IsEnabled() &&
+              type_ != extension_installed_bubble::kBundle)) {
     // If the toolbar redesign is enabled, all bubbles for extensions point to
-    // their toolbar action.
+    // their toolbar action. The exception is for bundles, for which there is no
+    // single associated extension.
     BrowserActionsController* controller =
         [[window->cocoa_controller() toolbarController]
             browserActionsController];

chrome/browser/ui/location_bar/location_bar_browsertest.cc

@@ -54,6 +54,7 @@ class LocationBarBrowserTest : public ExtensionBrowserTest {
 
  private:
   scoped_ptr<extensions::FeatureSwitch::ScopedOverride> enable_override_;
+  scoped_ptr<extensions::FeatureSwitch::ScopedOverride> enable_redesign_;
 
   DISALLOW_COPY_AND_ASSIGN(LocationBarBrowserTest);
 };
@@ -63,6 +64,10 @@ void LocationBarBrowserTest::SetUpCommandLine(base::CommandLine* command_line) {
   // enable the switch.
   enable_override_.reset(new extensions::FeatureSwitch::ScopedOverride(
       extensions::FeatureSwitch::enable_override_bookmarks_ui(), true));
+  // For testing page actions in the location bar, we also have to be sure to
+  // *not* have the redesign turned on.
+  enable_redesign_.reset(new extensions::FeatureSwitch::ScopedOverride(
+      extensions::FeatureSwitch::extension_action_redesign(), false));
   ExtensionBrowserTest::SetUpCommandLine(command_line);
 }
 

chrome/browser/ui/toolbar/browser_actions_bar_browsertest.cc

@@ -60,8 +60,12 @@ BrowserActionsBarBrowserTest::~BrowserActionsBarBrowserTest() {
 
 void BrowserActionsBarBrowserTest::SetUpCommandLine(
     base::CommandLine* command_line) {
-  ToolbarActionsBar::disable_animations_for_testing_ = true;
   ExtensionBrowserTest::SetUpCommandLine(command_line);
+  ToolbarActionsBar::disable_animations_for_testing_ = true;
+  // These tests are deliberately testing behavior without the redesign.
+  // Forcefully disable it.
+  override_redesign_.reset(new extensions::FeatureSwitch::ScopedOverride(
+      extensions::FeatureSwitch::extension_action_redesign(), false));
 }
 
 void BrowserActionsBarBrowserTest::SetUpOnMainThread() {
@@ -110,9 +114,9 @@ BrowserActionsBarRedesignBrowserTest::~BrowserActionsBarRedesignBrowserTest() {
 void BrowserActionsBarRedesignBrowserTest::SetUpCommandLine(
     base::CommandLine* command_line) {
   BrowserActionsBarBrowserTest::SetUpCommandLine(command_line);
-  enable_redesign_.reset(new extensions::FeatureSwitch::ScopedOverride(
-      extensions::FeatureSwitch::extension_action_redesign(),
-      true));
+  // Override to force the redesign.
+  override_redesign_.reset(new extensions::FeatureSwitch::ScopedOverride(
+      extensions::FeatureSwitch::extension_action_redesign(), true));
 }
 
 // Test the basic functionality.

chrome/browser/ui/toolbar/browser_actions_bar_browsertest.h

@@ -48,6 +48,10 @@ class BrowserActionsBarBrowserTest : public ExtensionBrowserTest {
     return extension_c_.get();
   }
 
+ protected:
+  // Enable or disable the feature redesign switch.
+  scoped_ptr<extensions::FeatureSwitch::ScopedOverride> override_redesign_;
+
  private:
   scoped_ptr<BrowserActionTestUtil> browser_actions_bar_;
 
@@ -72,9 +76,6 @@ class BrowserActionsBarRedesignBrowserTest
   void SetUpCommandLine(base::CommandLine* command_line) override;
 
  private:
-  // Enable the feature redesign switch.
-  scoped_ptr<extensions::FeatureSwitch::ScopedOverride> enable_redesign_;
-
   DISALLOW_COPY_AND_ASSIGN(BrowserActionsBarRedesignBrowserTest);
 };
 

chrome/browser/ui/views/location_bar/page_action_image_view_interactive_uitest.cc

@@ -14,6 +14,7 @@
 #include "chrome/browser/ui/views/location_bar/page_action_with_badge_view.h"
 #include "chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.h"
 #include "extensions/common/constants.h"
+#include "extensions/common/feature_switch.h"
 #include "extensions/test/extension_test_message_listener.h"
 #include "ui/base/test/ui_controls.h"
 #include "ui/gfx/image/canvas_image_source.h"
@@ -21,7 +22,23 @@
 #include "ui/gfx/image/image_skia_operations.h"
 #include "ui/views/controls/menu/menu_controller.h"
 
-using PageActionImageViewInteractiveUITest = ExtensionBrowserTest;
+class PageActionImageViewInteractiveUITest : public ExtensionBrowserTest {
+ protected:
+  PageActionImageViewInteractiveUITest() {}
+  ~PageActionImageViewInteractiveUITest() override {}
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    ExtensionBrowserTest::SetUpCommandLine(command_line);
+    // Testing page action-specific UI means we need to disable the redesign.
+    disable_redesign_.reset(new extensions::FeatureSwitch::ScopedOverride(
+        extensions::FeatureSwitch::extension_action_redesign(), false));
+  }
+
+ private:
+  scoped_ptr<extensions::FeatureSwitch::ScopedOverride> disable_redesign_;
+
+  DISALLOW_COPY_AND_ASSIGN(PageActionImageViewInteractiveUITest);
+};
 
 // An ImageSkia source that will do nothing. We need this because we need a
 // blank canvas at a certain size, and that can't be done by just using a null

testing/variations/fieldtrial_testing_config_chromeos.json

@@ -19,6 +19,11 @@
             "group_name": "Button"
         }
     ],
+    "ExtensionActionRedesign": [
+        {
+            "group_name": "Enabled"
+        }
+    ],
     "PluginPowerSaver": [
         {
             "group_name": "Enabled"

testing/variations/fieldtrial_testing_config_linux.json

@@ -29,6 +29,11 @@
             "group_name": "Enabled"
         }
     ],
+    "ExtensionActionRedesign": [
+        {
+            "group_name": "Enabled"
+        }
+    ],
     "InstanceID": [
         {
             "group_name": "Enabled"

testing/variations/fieldtrial_testing_config_mac.json

@@ -24,6 +24,11 @@
             "group_name": "Button"
         }
     ],
+    "ExtensionActionRedesign": [
+        {
+            "group_name": "Enabled"
+        }
+    ],
     "ExtensionDeveloperModeWarning": [
         {
             "group_name": "Enabled"

testing/variations/fieldtrial_testing_config_win.json

@@ -44,6 +44,11 @@
             "group_name": "Enabled"
         }
     ],
+    "ExtensionActionRedesign": [
+        {
+            "group_name": "Enabled"
+        }
+    ],
     "ExtensionContentVerification": [
         {
             "group_name": "Enforce"