Remove the implicit wrench menu items for browser actions.

Also, allow browser actions with no initial icons, and add
some better tests.

BUG=24379,24671

Review URL: http://codereview.chromium.org/276010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@29095 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 150 insertions, 135 deletions

diff --git a/chrome/browser/extensions/browser_action_apitest.cc b/chrome/browser/extensions/browser_action_apitest.cc
new file mode 100644
index 0000000..692e74e
--- /dev/null
+++ b/chrome/browser/extensions/browser_action_apitest.cc
@@ -0,0 +1,86 @@
+// Copyright (c) 2009 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 "chrome/browser/browser.h"
+#include "chrome/browser/browser_window.h"
+#include "chrome/browser/extensions/extension_apitest.h"
+#include "chrome/browser/extensions/extension_browser_event_router.h"
+#include "chrome/browser/extensions/extension_tabs_module.h"
+#include "chrome/browser/extensions/extensions_service.h"
+#include "chrome/browser/profile.h"
+#include "chrome/browser/tab_contents/tab_contents.h"
+#include "chrome/browser/views/browser_actions_container.h"
+#include "chrome/browser/views/toolbar_view.h"
+#include "chrome/common/extensions/extension_action.h"
+#include "chrome/test/ui_test_utils.h"
+
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, BrowserAction) {
+  StartHTTPServer();
+  ASSERT_TRUE(RunExtensionTest("browser_action")) << message_;
+
+  // Test that there is a browser action in the toolbar.
+  BrowserActionsContainer* browser_actions =
+      browser()->window()->GetBrowserWindowTesting()->GetToolbarView()->
+      browser_actions();
+  ASSERT_EQ(1, browser_actions->num_browser_actions());
+
+  // Tell the extension to update the browser action state.
+  ResultCatcher catcher;
+  ExtensionsService* service = browser()->profile()->GetExtensionsService();
+  Extension* extension = service->extensions()->at(0);
+  ui_test_utils::NavigateToURL(browser(),
+      GURL(extension->GetResourceURL("update.html")));
+  ASSERT_TRUE(catcher.GetNextResult());
+
+  // Test that we received the changes.
+  ExtensionActionState* action_state = extension->browser_action_state();
+  ASSERT_EQ("Modified", action_state->title());
+  ASSERT_EQ(1, action_state->icon_index());
+  ASSERT_EQ("badge", action_state->badge_text());
+  ASSERT_EQ(SkColorSetARGB(255, 255, 255, 255),
+            action_state->badge_background_color());
+
+  // Simulate the browser action being clicked.
+  ui_test_utils::NavigateToURL(browser(),
+      GURL("http://localhost:1337/files/extensions/test_file.txt"));
+
+  ExtensionAction* browser_action = service->GetBrowserActions(false)[0];
+  int window_id = ExtensionTabUtil::GetWindowId(browser());
+  ExtensionBrowserEventRouter::GetInstance()->BrowserActionExecuted(
+      browser()->profile(), browser_action->extension_id(), browser());
+
+  // Verify the command worked.
+  TabContents* tab = browser()->GetSelectedTabContents();
+  bool result = false;
+  ui_test_utils::ExecuteJavaScriptAndExtractBool(
+      tab->render_view_host(), L"",
+      L"setInterval(function(){"
+      L"  if(document.body.bgColor == 'red'){"
+      L"    window.domAutomationController.send(true)}}, 100)",
+      &result);
+  ASSERT_TRUE(result);
+}
+
+
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, DynamicBrowserAction) {
+  ASSERT_TRUE(RunExtensionTest("browser_action_no_icon")) << message_;
+
+  // Test that there is a browser action in the toolbar.
+  BrowserActionsContainer* browser_actions =
+      browser()->window()->GetBrowserWindowTesting()->GetToolbarView()->
+      browser_actions();
+  ASSERT_EQ(1, browser_actions->num_browser_actions());
+
+  // Tell the extension to update the browser action state.
+  ResultCatcher catcher;
+  ExtensionsService* service = browser()->profile()->GetExtensionsService();
+  Extension* extension = service->extensions()->at(0);
+  ui_test_utils::NavigateToURL(browser(),
+      GURL(extension->GetResourceURL("update.html")));
+  ASSERT_TRUE(catcher.GetNextResult());
+
+  // Test that we received the changes.
+  ExtensionActionState* action_state = extension->browser_action_state();
+  ASSERT_TRUE(action_state->icon());
+}
diff --git a/chrome/browser/extensions/browser_action_test.cc b/chrome/browser/extensions/browser_action_test.cc
deleted file mode 100644
index dca093c..0000000
--- a/chrome/browser/extensions/browser_action_test.cc
+++ /dev/null
@@ -1,86 +0,0 @@
-// Copyright (c) 2009 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 "chrome/browser/browser.h"
-#include "chrome/browser/browser_window.h"
-#include "chrome/browser/extensions/extension_browsertest.h"
-#include "chrome/browser/extensions/extensions_service.h"
-#include "chrome/browser/profile.h"
-#include "chrome/browser/tab_contents/tab_contents.h"
-#include "chrome/browser/views/browser_actions_container.h"
-#include "chrome/browser/views/toolbar_view.h"
-#include "chrome/common/extensions/extension_action.h"
-#include "chrome/test/ui_test_utils.h"
-
-#if defined(OS_LINUX)
-#include "chrome/browser/gtk/view_id_util.h"
-#endif
-
-static void CheckButtonCount(Browser* browser, const int expected_count) {
-#if defined(OS_WIN)
-  BrowserActionsContainer* browser_actions =
-      browser->window()->GetBrowserWindowTesting()->GetToolbarView()->
-      browser_actions();
-  int num_buttons = browser_actions->num_browser_actions();
-#elif defined(OS_LINUX)
-  GtkWidget* widget = ViewIDUtil::GetWidget(
-      GTK_WIDGET(browser->window()->GetNativeHandle()),
-      VIEW_ID_BROWSER_ACTION_TOOLBAR);
-  ASSERT_TRUE(widget);
-  GList* children = gtk_container_get_children(GTK_CONTAINER(widget));
-  int num_buttons = g_list_length(children);
-  g_list_free(children);
-#endif
-
-  EXPECT_EQ(expected_count, num_buttons);
-}
-
-static void TestAction(Browser* browser) {
-  // Navigate to a page we have permission to modify.
-  ui_test_utils::NavigateToURL(browser,
-      GURL("http://localhost:1337/files/extensions/test_file.txt"));
-
-  // Send the command.  Note that this only works for non-popup actions, so
-  // we specify |false|.
-  ExtensionsService* service = browser->profile()->GetExtensionsService();
-  browser->ExecuteCommand(service->GetBrowserActions(false)[0]->command_id());
-
-  // Verify the command worked.
-  TabContents* tab = browser->GetSelectedTabContents();
-  bool result = false;
-  ui_test_utils::ExecuteJavaScriptAndExtractBool(
-      tab->render_view_host(), L"",
-      L"setInterval(function(){"
-      L"  if(document.body.bgColor == 'red'){"
-      L"    window.domAutomationController.send(true)}}, 100)",
-      &result);
-  ASSERT_TRUE(result);
-}
-
-// Crashes frequently on Linux. See http://crbug.com/24802.
-IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, DISABLED_BrowserAction) {
-  StartHTTPServer();
-
-  ASSERT_TRUE(LoadExtension(
-      test_data_dir_.AppendASCII("samples")
-                    .AppendASCII("make_page_red")));
-
-  // Test that there is a browser action in the toolbar.
-  CheckButtonCount(browser(), 1);
-
-  TestAction(browser());
-}
-
-IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, BrowserActionNoIcon) {
-  StartHTTPServer();
-
-  ASSERT_TRUE(LoadExtension(
-      test_data_dir_.AppendASCII("samples")
-                    .AppendASCII("make_page_red_no_icon")));
-
-  // Test that there is a *not* a browser action in the toolbar.
-  CheckButtonCount(browser(), 0);
-
-  TestAction(browser());
-}
diff --git a/chrome/browser/extensions/extension_apitest.cc b/chrome/browser/extensions/extension_apitest.cc
index 0a8875d..65f1058 100644
--- a/chrome/browser/extensions/extension_apitest.cc
+++ b/chrome/browser/extensions/extension_apitest.cc
@@ -12,35 +12,14 @@ namespace {
 static const int kTimeoutMs = 60 * 1000;  // 1 minute
 };
 
-// Load an extension and wait for it to notify of PASSED or FAILED.
-bool ExtensionApiTest::RunExtensionTest(const char* extension_name) {
-  // Note the inner scope here. The |registrar| will fall out of scope and
-  // remove listeners *before* the call to WaitForPassFail() below.
-  {
-    LOG(INFO) << "Running ExtensionApiTest with: " << extension_name;
-    NotificationRegistrar registrar;
-    registrar.Add(this, NotificationType::EXTENSION_TEST_PASSED,
-                  NotificationService::AllSources());
-    registrar.Add(this, NotificationType::EXTENSION_TEST_FAILED,
-                  NotificationService::AllSources());
-
-    if (!LoadExtension(test_data_dir_.AppendASCII(extension_name))) {
-      message_ = "Failed to load extension.";
-      return false;
-    }
-  }
-
-  // TODO(erikkay) perhaps we shouldn't do this implicitly.
-  return WaitForPassFail();
+ExtensionApiTest::ResultCatcher::ResultCatcher() {
+  registrar_.Add(this, NotificationType::EXTENSION_TEST_PASSED,
+                 NotificationService::AllSources());
+  registrar_.Add(this, NotificationType::EXTENSION_TEST_FAILED,
+                 NotificationService::AllSources());
 }
 
-bool ExtensionApiTest::WaitForPassFail() {
-  NotificationRegistrar registrar;
-  registrar.Add(this, NotificationType::EXTENSION_TEST_PASSED,
-                NotificationService::AllSources());
-  registrar.Add(this, NotificationType::EXTENSION_TEST_FAILED,
-                NotificationService::AllSources());
-
+bool ExtensionApiTest::ResultCatcher::GetNextResult() {
   // Depending on the tests, multiple results can come in from a single call
   // to RunMessageLoop(), so we maintain a queue of results and just pull them
   // off as the test calls this, going to the run loop only when the queue is
@@ -61,14 +40,9 @@ bool ExtensionApiTest::WaitForPassFail() {
   return false;
 }
 
-void ExtensionApiTest::SetUpCommandLine(CommandLine* command_line) {
-  ExtensionBrowserTest::SetUpCommandLine(command_line);
-  test_data_dir_ = test_data_dir_.AppendASCII("api_test");
-}
-
-void ExtensionApiTest::Observe(NotificationType type,
-                               const NotificationSource& source,
-                               const NotificationDetails& details) {
+void ExtensionApiTest::ResultCatcher::Observe(
+    NotificationType type, const NotificationSource& source,
+    const NotificationDetails& details) {
   switch (type.value) {
     case NotificationType::EXTENSION_TEST_PASSED:
       std::cout << "Got EXTENSION_TEST_PASSED notification.\n";
@@ -85,6 +59,29 @@ void ExtensionApiTest::Observe(NotificationType type,
       break;
 
     default:
-      ExtensionBrowserTest::Observe(type, source, details);
+      NOTREACHED();
+  }
+}
+
+// Load an extension and wait for it to notify of PASSED or FAILED.
+bool ExtensionApiTest::RunExtensionTest(const char* extension_name) {
+  ResultCatcher catcher;
+
+  LOG(INFO) << "Running ExtensionApiTest with: " << extension_name;
+  if (!LoadExtension(test_data_dir_.AppendASCII(extension_name))) {
+    message_ = "Failed to load extension.";
+    return false;
+  }
+
+  if (!catcher.GetNextResult()) {
+    message_ = catcher.message();
+    return false;
+  } else {
+    return true;
   }
 }
+
+void ExtensionApiTest::SetUpCommandLine(CommandLine* command_line) {
+  ExtensionBrowserTest::SetUpCommandLine(command_line);
+  test_data_dir_ = test_data_dir_.AppendASCII("api_test");
+}
diff --git a/chrome/browser/extensions/extension_apitest.h b/chrome/browser/extensions/extension_apitest.h
index 466735d..01ada82 100644
--- a/chrome/browser/extensions/extension_apitest.h
+++ b/chrome/browser/extensions/extension_apitest.h
@@ -19,25 +19,42 @@
 
 class ExtensionApiTest : public ExtensionBrowserTest {
  protected:
+  // Helper class that observes tests failing or passing. Observation starts when
+  // the class is constructed. Get the next result by calling GetNextResult() and
+  // message() if GetNextResult() return false. If there are no results, this
+  // method will pump the UI message loop until one is received.
+  class ResultCatcher : public NotificationObserver {
+   public:
+    ResultCatcher();
+
+    // Pumps the UI loop until a notification is received that an API test
+    // succeeded or failed. Returns true if the test succeeded, false otherwise.
+    bool GetNextResult();
+
+    const std::string& message() { return message_; }
+
+   private:
+    virtual void Observe(NotificationType type, const NotificationSource& source,
+                         const NotificationDetails& details);
+
+    NotificationRegistrar registrar_;
+
+    // A sequential list of pass/fail notifications from the test extension(s).
+    std::deque<bool> results_;
+
+    // If it failed, what was the error message?
+    std::deque<std::string> messages_;
+    std::string message_;
+  };
+
   // Load |extension_name| and wait for pass / fail notification.
   // |extension_name| is a directory in "test/data/extensions/api_test".
   bool RunExtensionTest(const char* extension_name);
 
-  // Reset |completed_| and wait for a new pass / fail notification.
-  bool WaitForPassFail();
-
   // All extensions tested by ExtensionApiTest are in the "api_test" dir.
   virtual void SetUpCommandLine(CommandLine* command_line);
 
-  // NotificationObserver
-  void Observe(NotificationType type, const NotificationSource& source,
-               const NotificationDetails& details);
-
-  // A sequential list of pass/fail notifications from the test extension(s).
-  std::deque<bool> results_;
-
   // If it failed, what was the error message?
-  std::deque<std::string> messages_;
   std::string message_;
 };
 
diff --git a/chrome/browser/extensions/extension_browser_actions_api.cc b/chrome/browser/extensions/extension_browser_actions_api.cc
index d2cb604..e7d875e 100644
--- a/chrome/browser/extensions/extension_browser_actions_api.cc
+++ b/chrome/browser/extensions/extension_browser_actions_api.cc
@@ -42,6 +42,7 @@ bool BrowserActionSetIconFunction::RunImpl() {
     EXTENSION_FUNCTION_VALIDATE(
         static_cast<DictionaryValue*>(args_)->GetInteger(
             L"iconIndex", &icon_index));
+
     if (icon_index < 0 ||
         static_cast<size_t>(icon_index) >=
             extension->browser_action()->icon_paths().size()) {
diff --git a/chrome/browser/extensions/extension_browser_actions_api.h b/chrome/browser/extensions/extension_browser_actions_api.h
index 1becc70..8d92b8a 100755
--- a/chrome/browser/extensions/extension_browser_actions_api.h
+++ b/chrome/browser/extensions/extension_browser_actions_api.h
@@ -9,12 +9,12 @@
 
 class BrowserActionSetIconFunction : public SyncExtensionFunction {
   virtual bool RunImpl();
-  DECLARE_EXTENSION_FUNCTION_NAME("browserAction.setName")
+  DECLARE_EXTENSION_FUNCTION_NAME("browserAction.setIcon")
 };
 
 class BrowserActionSetTitleFunction : public SyncExtensionFunction {
   virtual bool RunImpl();
-  DECLARE_EXTENSION_FUNCTION_NAME("browserAction.setIcon")
+  DECLARE_EXTENSION_FUNCTION_NAME("browserAction.setTitle")
 };
 
 class BrowserActionSetBadgeTextFunction : public SyncExtensionFunction {