Revert of [Extensions Toolbar] Add a finch config for the redesign (p…

…atchset chromium#2 id:60001 of https://codereview.chromium.org/1363463002/ ) Reason for revert: This is causing time out for XP Tests: browser_tests: DeclarativeContentApiTest.ShowPageActionWithoutPageAction. Original issue's description: > [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 > > Committed: https://crrev.com/ca056fb34f9003262f28061856042ed87ed865c4 > Cr-Commit-Position: refs/heads/master@{#350296} TBR=finnur@chromium.org,thakis@chromium.org,rdevlin.cronin@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=533101 Review URL: https://codereview.chromium.org/1363943002 Cr-Commit-Position: refs/heads/master@{#350341}
marcosholgado · Sep 23, 2015 · c059a0e · c059a0e
1 parent 765035f
commit c059a0e
Show file tree

Hide file tree

Showing 14 changed files with 70 additions and 156 deletions.
diff --git a/chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc b/chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc
@@ -21,7 +21,6 @@
 #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"
 
@@ -644,65 +643,32 @@ 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);
 
-  std::string extension_id;
+  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 script =
-      "setRules([{\n"
-      "  conditions: [new PageStateMatcher({\n"
-      "                   pageUrl: {hostPrefix: \"test\"}})],\n"
-      "  actions: [new ShowPageAction()]\n"
-      "}], 'test_rule');\n";
+  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(
-        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));
-  }
+  EXPECT_EQ(NULL,
+            ExtensionActionManager::Get(browser()->profile())->
+                GetPageAction(*extension));
+  EXPECT_EQ(0u, extension_action_test_util::GetVisiblePageActionCount(tab));
 }
 
 IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest,

diff --git a/chrome/browser/extensions/extension_action.cc b/chrome/browser/extensions/extension_action.cc
@@ -256,20 +256,15 @@ gfx::Image ExtensionAction::GetDefaultIconImage() const {
   if (default_icon_image_)
     return default_icon_image_->image();
 
-  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();
-    }
+  // 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_);
   }
 
-  return placeholder_icon_image_;
+  return GetDefaultIcon();
 }
 
 bool ExtensionAction::HasPopupUrl(int tab_id) const {

diff --git a/chrome/browser/extensions/extension_action.h b/chrome/browser/extensions/extension_action.h
@@ -13,7 +13,6 @@
 #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;
 
@@ -288,11 +287,6 @@ 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_;

diff --git a/chrome/browser/extensions/lazy_background_page_apitest.cc b/chrome/browser/extensions/lazy_background_page_apitest.cc
@@ -9,7 +9,6 @@
 #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"
@@ -42,8 +41,8 @@
 
 using bookmarks::BookmarkModel;
 using bookmarks::BookmarkNode;
-
-namespace extensions {
+using extensions::Extension;
+using extensions::ResultCatcher;
 
 namespace {
 
@@ -53,11 +52,12 @@ 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 ExtensionRegistryObserver {
+class LoadedIncognitoObserver : public extensions::ExtensionRegistryObserver {
  public:
   explicit LoadedIncognitoObserver(Profile* profile)
       : profile_(profile), extension_registry_observer_(this) {
-    extension_registry_observer_.Add(ExtensionRegistry::Get(profile_));
+    extension_registry_observer_.Add(
+        extensions::ExtensionRegistry::Get(profile_));
   }
 
   void Wait() {
@@ -67,16 +67,18 @@ class LoadedIncognitoObserver : public ExtensionRegistryObserver {
   }
 
  private:
-  void OnExtensionUnloaded(content::BrowserContext* browser_context,
-                           const Extension* extension,
-                           UnloadedExtensionInfo::Reason reason) override {
+  void OnExtensionUnloaded(
+      content::BrowserContext* browser_context,
+      const Extension* extension,
+      extensions::UnloadedExtensionInfo::Reason reason) override {
     original_complete_.reset(new LazyBackgroundObserver(profile_));
     incognito_complete_.reset(
         new LazyBackgroundObserver(profile_->GetOffTheRecordProfile()));
   }
 
   Profile* profile_;
-  ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>
+  ScopedObserver<extensions::ExtensionRegistry,
+                 extensions::ExtensionRegistryObserver>
       extension_registry_observer_;
   scoped_ptr<LazyBackgroundObserver> original_complete_;
   scoped_ptr<LazyBackgroundObserver> incognito_complete_;
@@ -92,8 +94,8 @@ class LazyBackgroundPageApiTest : public ExtensionApiTest {
   void SetUpInProcessBrowserTestFixture() override {
     ExtensionApiTest::SetUpInProcessBrowserTestFixture();
     // Set shorter delays to prevent test timeouts.
-    ProcessManager::SetEventPageIdleTimeForTesting(1);
-    ProcessManager::SetEventPageSuspendingTimeForTesting(1);
+    extensions::ProcessManager::SetEventPageIdleTimeForTesting(1);
+    extensions::ProcessManager::SetEventPageSuspendingTimeForTesting(1);
   }
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
@@ -119,7 +121,8 @@ 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) {
-    ProcessManager* pm = ProcessManager::Get(browser()->profile());
+    extensions::ProcessManager* pm =
+        extensions::ProcessManager::Get(browser()->profile());
     return pm->GetBackgroundHostForExtension(extension_id);
   }
 
@@ -188,9 +191,9 @@ IN_PROC_BROWSER_TEST_F(LazyBackgroundPageApiTest, BroadcastEvent) {
 
   // Page action is shown.
   WaitForPageActionVisibilityChangeTo(num_page_actions + 1);
-  EXPECT_EQ(static_cast<size_t>(num_page_actions + 1),
-            extension_action_test_util::GetVisiblePageActionCount(
-                browser()->tab_strip_model()->GetActiveWebContents()));
+  EXPECT_EQ(num_page_actions + 1,
+            browser()->window()->GetLocationBar()->
+                GetLocationBarForTesting()->PageActionVisibleCount());
 }
 
 IN_PROC_BROWSER_TEST_F(LazyBackgroundPageApiTest, Filters) {
@@ -236,7 +239,8 @@ IN_PROC_BROWSER_TEST_F(LazyBackgroundPageApiTest, WaitForDialog) {
   EXPECT_TRUE(IsBackgroundPageAlive(extension->id()));
 
   // Close the dialog. The keep alive count is decremented.
-  ProcessManager* pm = ProcessManager::Get(browser()->profile());
+  extensions::ProcessManager* pm =
+      extensions::ProcessManager::Get(browser()->profile());
   int previous_keep_alive_count = pm->GetLazyKeepaliveCount(extension);
   dialog->CloseModalDialog();
   EXPECT_EQ(previous_keep_alive_count - 1,
@@ -291,8 +295,9 @@ IN_PROC_BROWSER_TEST_F(LazyBackgroundPageApiTest, WaitForRequest) {
   EXPECT_TRUE(catcher.GetNextResult()) << catcher.message();
 
   // Lazy Background Page still exists, because the extension started a request.
-  ProcessManager* pm = ProcessManager::Get(browser()->profile());
-  ExtensionHost* host =
+  extensions::ProcessManager* pm =
+      extensions::ProcessManager::Get(browser()->profile());
+  extensions::ExtensionHost* host =
       pm->GetBackgroundHostForExtension(last_loaded_extension_id());
   ASSERT_TRUE(host);
 
@@ -394,8 +399,10 @@ IN_PROC_BROWSER_TEST_F(LazyBackgroundPageApiTest, DISABLED_IncognitoSplitMode) {
   }
 
   // Lazy Background Page doesn't exist yet.
-  ProcessManager* pm = ProcessManager::Get(browser()->profile());
-  ProcessManager* pmi = ProcessManager::Get(incognito_browser->profile());
+  extensions::ProcessManager* pm =
+      extensions::ProcessManager::Get(browser()->profile());
+  extensions::ProcessManager* pmi =
+      extensions::ProcessManager::Get(incognito_browser->profile());
   EXPECT_FALSE(pm->GetBackgroundHostForExtension(last_loaded_extension_id()));
   EXPECT_FALSE(pmi->GetBackgroundHostForExtension(last_loaded_extension_id()));
 
@@ -481,7 +488,8 @@ IN_PROC_BROWSER_TEST_F(LazyBackgroundPageApiTest, ImpulseAddsCount) {
   ASSERT_TRUE(extension);
 
   // Lazy Background Page doesn't exist yet.
-  ProcessManager* pm = ProcessManager::Get(browser()->profile());
+  extensions::ProcessManager* pm =
+      extensions::ProcessManager::Get(browser()->profile());
   EXPECT_FALSE(pm->GetBackgroundHostForExtension(last_loaded_extension_id()));
   EXPECT_EQ(1, browser()->tab_strip_model()->count());
 
@@ -527,7 +535,8 @@ IN_PROC_BROWSER_TEST_F(LazyBackgroundPageApiTest, EventDispatchToTab) {
   ResultCatcher catcher;
   catcher.RestrictToBrowserContext(browser()->profile());
 
-  const Extension* extension = LoadExtensionAndWait("event_dispatch_to_tab");
+  const extensions::Extension* extension =
+      LoadExtensionAndWait("event_dispatch_to_tab");
 
   ExtensionTestMessageListener page_ready("ready", true);
   GURL page_url = extension->GetResourceURL("page.html");
@@ -621,5 +630,3 @@ IN_PROC_BROWSER_TEST_F(LazyBackgroundPageApiTest, OnSuspendUseStorageApi) {
 
 // TODO: background page with timer.
 // TODO: background page that interacts with popup.
-
-}  // namespace extensions
diff --git a/chrome/browser/extensions/location_bar_controller.cc b/chrome/browser/extensions/location_bar_controller.cc
@@ -30,7 +30,8 @@ LocationBarController::LocationBarController(
       should_show_page_actions_(
           !FeatureSwitch::extension_action_redesign()->IsEnabled()),
       extension_registry_observer_(this) {
-  extension_registry_observer_.Add(ExtensionRegistry::Get(browser_context_));
+  if (should_show_page_actions_)
+    extension_registry_observer_.Add(ExtensionRegistry::Get(browser_context_));
 }
 
 LocationBarController::~LocationBarController() {
@@ -92,7 +93,7 @@ std::vector<ExtensionAction*> LocationBarController::GetCurrentActions() {
 void LocationBarController::OnExtensionLoaded(
     content::BrowserContext* browser_context,
     const Extension* extension) {
-  if (should_show_page_actions_ && action_manager_->GetPageAction(*extension)) {
+  if (action_manager_->GetPageAction(*extension)) {
     ExtensionActionAPI::Get(browser_context)->
         NotifyPageActionsChanged(web_contents_);
   }
@@ -121,7 +122,7 @@ void LocationBarController::OnExtensionUnloaded(
     content::BrowserContext* browser_context,
     const Extension* extension,
     UnloadedExtensionInfo::Reason reason) {
-  if (should_show_page_actions_ && action_manager_->GetPageAction(*extension)) {
+  if (action_manager_->GetPageAction(*extension)) {
     ExtensionActionAPI::Get(browser_context)->
         NotifyPageActionsChanged(web_contents_);
   }

diff --git a/chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm b/chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm
@@ -234,12 +234,10 @@ - (NSPoint)calculateArrowPoint {
         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() &&
-              type_ != extension_installed_bubble::kBundle)) {
+             extensions::FeatureSwitch::extension_action_redesign()->
+                 IsEnabled()) {
     // If the toolbar redesign is enabled, all bubbles for extensions point to
-    // their toolbar action. The exception is for bundles, for which there is no
-    // single associated extension.
+    // their toolbar action.
     BrowserActionsController* controller =
         [[window->cocoa_controller() toolbarController]
             browserActionsController];

diff --git a/chrome/browser/ui/location_bar/location_bar_browsertest.cc b/chrome/browser/ui/location_bar/location_bar_browsertest.cc
@@ -54,7 +54,6 @@ class LocationBarBrowserTest : public ExtensionBrowserTest {
 
  private:
   scoped_ptr<extensions::FeatureSwitch::ScopedOverride> enable_override_;
-  scoped_ptr<extensions::FeatureSwitch::ScopedOverride> enable_redesign_;
 
   DISALLOW_COPY_AND_ASSIGN(LocationBarBrowserTest);
 };
@@ -64,10 +63,6 @@ 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);
 }
 

diff --git a/chrome/browser/ui/toolbar/browser_actions_bar_browsertest.cc b/chrome/browser/ui/toolbar/browser_actions_bar_browsertest.cc
@@ -60,12 +60,8 @@ BrowserActionsBarBrowserTest::~BrowserActionsBarBrowserTest() {
 
 void BrowserActionsBarBrowserTest::SetUpCommandLine(
     base::CommandLine* command_line) {
-  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));
+  ExtensionBrowserTest::SetUpCommandLine(command_line);
 }
 
 void BrowserActionsBarBrowserTest::SetUpOnMainThread() {
@@ -114,9 +110,9 @@ BrowserActionsBarRedesignBrowserTest::~BrowserActionsBarRedesignBrowserTest() {
 void BrowserActionsBarRedesignBrowserTest::SetUpCommandLine(
     base::CommandLine* command_line) {
   BrowserActionsBarBrowserTest::SetUpCommandLine(command_line);
-  // Override to force the redesign.
-  override_redesign_.reset(new extensions::FeatureSwitch::ScopedOverride(
-      extensions::FeatureSwitch::extension_action_redesign(), true));
+  enable_redesign_.reset(new extensions::FeatureSwitch::ScopedOverride(
+      extensions::FeatureSwitch::extension_action_redesign(),
+      true));
 }
 
 // Test the basic functionality.