Revert 29861 since this fail on the interactive

linux dbg bot, for some weird reason...

Page actions that don't specify an icon (ie.
have a spelling error in the manifest, such as
icon instead of icons/default_icon) caused a
crash when they try to display their icon.

We now check when the extension tries to enable
the page action whether there are any icons to
display. If not, we don't proceed and log an
error to the console.

TBR=nsylvain
BUG=25562
TEST=Covered by browser test.
Review URL: http://codereview.chromium.org/316018

TBR=finnur@chromium.org
Review URL: http://codereview.chromium.org/327007

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

8 files changed, 18 insertions, 91 deletions

diff --git a/chrome/browser/extensions/extension_browsertests_misc.cc b/chrome/browser/extensions/extension_browsertests_misc.cc
index 9490510..e991926 100644
--- a/chrome/browser/extensions/extension_browsertests_misc.cc
+++ b/chrome/browser/extensions/extension_browsertests_misc.cc
@@ -207,12 +207,6 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, TabContents) {
 #if defined(OS_WIN) || defined(OS_LINUX)
 // Tests that we can load page actions in the Omnibox.
 IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, PageAction) {
-  // This page action will not show an icon, since it doesn't specify one but
-  // is included here to test for a crash (http://crbug.com/25562).
-  ASSERT_TRUE(LoadExtension(
-      test_data_dir_.AppendASCII("browsertest")
-                    .AppendASCII("crash_25562")));
-
   ASSERT_TRUE(LoadExtension(
       test_data_dir_.AppendASCII("samples")
                     .AppendASCII("subscribe_page_action")));
diff --git a/chrome/browser/extensions/extension_page_actions_module.cc b/chrome/browser/extensions/extension_page_actions_module.cc
index 4738891..743e0a5 100644
--- a/chrome/browser/extensions/extension_page_actions_module.cc
+++ b/chrome/browser/extensions/extension_page_actions_module.cc
@@ -21,12 +21,12 @@ namespace keys = extension_page_actions_module_constants;
 
 namespace {
 // Errors.
+const char kNoExtensionError[] = "No extension with id: *.";
 const char kNoTabError[] = "No tab with id: *.";
 const char kNoPageActionError[] =
     "This extension has no page action specified.";
 const char kUrlNotActiveError[] = "This url is no longer active: *.";
 const char kIconIndexOutOfBounds[] = "Page action icon index out of bounds.";
-const char kNoIconSpecified[] = "Page action has no icons to show.";
 }
 
 // TODO(EXTENSIONS_DEPRECATED): obsolete API.
@@ -56,19 +56,6 @@ bool PageActionFunction::SetPageActionEnabled(bool enable) {
     }
   }
 
-  const ExtensionAction* page_action =
-      dispatcher()->GetExtension()->page_action();
-  if (!page_action) {
-    error_ = kNoPageActionError;
-    return false;
-  }
-
-  if (icon_id < 0 ||
-      static_cast<size_t>(icon_id) >= page_action->icon_paths().size()) {
-    error_ = (icon_id == 0) ? kNoIconSpecified : kIconIndexOutOfBounds;
-    return false;
-  }
-
   // Find the TabContents that contains this tab id.
   TabContents* contents = NULL;
   ExtensionTabUtil::GetTabById(tab_id, profile(), NULL, NULL, &contents, NULL);
@@ -85,6 +72,22 @@ bool PageActionFunction::SetPageActionEnabled(bool enable) {
     return false;
   }
 
+  // Find our extension.
+  Extension* extension = NULL;
+  ExtensionsService* service = profile()->GetExtensionsService();
+  extension = service->GetExtensionById(extension_id());
+  if (!extension) {
+    error_ = ExtensionErrorUtils::FormatErrorMessage(kNoExtensionError,
+                                                     extension_id());
+    return false;
+  }
+
+  const ExtensionAction* page_action = extension->page_action();
+  if (!page_action) {
+    error_ = kNoPageActionError;
+    return false;
+  }
+
   // Set visibility and broadcast notifications that the UI should be updated.
   contents->SetPageActionEnabled(page_action, enable, title, icon_id);
   contents->PageActionStateChanged();
diff --git a/chrome/browser/gtk/location_bar_view_gtk.cc b/chrome/browser/gtk/location_bar_view_gtk.cc
index 382563a..8648099 100644
--- a/chrome/browser/gtk/location_bar_view_gtk.cc
+++ b/chrome/browser/gtk/location_bar_view_gtk.cc
@@ -409,15 +409,6 @@ void LocationBarViewGtk::UpdatePageActions() {
   if (profile_->GetExtensionsService())
       page_actions = profile_->GetExtensionsService()->GetPageActions();
 
-  // Page actions can be created without an icon, so make sure we count only
-  // those that have been given an icon.
-  for (size_t i = 0; i < page_actions.size();) {
-    if (page_actions[i]->icon_paths().empty())
-      page_actions.erase(page_actions.begin() + i);
-    else
-      ++i;
-  }
-
   // Initialize on the first call, or re-inialize if more extensions have been
   // loaded or added after startup.
   if (page_actions.size() != page_action_views_.size()) {
diff --git a/chrome/browser/views/location_bar_view.cc b/chrome/browser/views/location_bar_view.cc
index dec21b8..9ca58e8b 100644
--- a/chrome/browser/views/location_bar_view.cc
+++ b/chrome/browser/views/location_bar_view.cc
@@ -92,7 +92,7 @@ static std::wstring GetKeywordName(Profile* profile,
 // A container for the PageActionImageView plus its badge.
 class LocationBarView::PageActionWithBadgeView : public views::View {
  public:
-  explicit PageActionWithBadgeView(PageActionImageView* image_view);
+  PageActionWithBadgeView(PageActionImageView* image_view);
 
   PageActionImageView* image_view() { return image_view_; }
 
@@ -698,15 +698,6 @@ void LocationBarView::RefreshPageActionViews() {
   if (profile_->GetExtensionsService())
     page_actions = profile_->GetExtensionsService()->GetPageActions();
 
-  // Page actions can be created without an icon, so make sure we count only
-  // those that have been given an icon.
-  for (size_t i = 0; i < page_actions.size();) {
-    if (page_actions[i]->icon_paths().empty())
-      page_actions.erase(page_actions.begin() + i);
-    else
-      ++i;
-  }
-
   // On startup we sometimes haven't loaded any extensions. This makes sure
   // we catch up when the extensions (and any page actions) load.
   if (page_actions.size() != page_action_views_.size()) {
diff --git a/chrome/test/data/extensions/browsertest/crash_25562/background.html b/chrome/test/data/extensions/browsertest/crash_25562/background.html
deleted file mode 100644
index f27aba8..0000000
--- a/chrome/test/data/extensions/browsertest/crash_25562/background.html
+++ /dev/null
@@ -1,28 +0,0 @@
-<html>
-<script>
-  // The Page Action ID.
-  var pageActionId = "TestId";
-
-  // The window this Page Action is associated with.
-  var windowId = -1;
-
-  // The TabId this Page Action is associated with.
-  var tabId = -1;
-
-  // The URL of the page on build.chromium.org.
-  var pageUrl = "";
-
-  chrome.self.onConnect.addListener(function(port) {
-    windowId = port.tab.windowId;
-    tabId = port.tab.id;
-    pageUrl = port.tab.url;
-
-    port.onMessage.addListener(function(mybool) {
-      // Let Chrome know that the PageAction needs to be enabled for this tabId
-      // and for the url of this page.
-      chrome.pageActions.enableForTab(pageActionId,
-                                      {tabId: tabId, url: pageUrl});
-    });
-  });
-</script>
-</html>
diff --git a/chrome/test/data/extensions/browsertest/crash_25562/chrome-16.png b/chrome/test/data/extensions/browsertest/crash_25562/chrome-16.png
deleted file mode 100644
index a3a4f3a..0000000
--- a/chrome/test/data/extensions/browsertest/crash_25562/chrome-16.png
+++ /dev/null
diff --git a/chrome/test/data/extensions/browsertest/crash_25562/manifest.json b/chrome/test/data/extensions/browsertest/crash_25562/manifest.json
deleted file mode 100644
index 9d9e8fb..0000000
--- a/chrome/test/data/extensions/browsertest/crash_25562/manifest.json
+++ /dev/null
@@ -1,17 +0,0 @@
-{
-   "background_page": "background.html",
-   "content_scripts": [ {
-      "js": [ "script.js" ],
-      "matches": [ "http://*/*" ]
-   } ],
-   "description": "A page action with incorrect icon specification",
-   "name": "Test extension",
-   "page_actions": [ {
-      "icon": "chrome-16.png",
-      "id": "TestId",
-      "name": "Page action name",
-      "tooltip": "Page action tooltip"
-   } ],
-   "permissions": [ "http://*/*" ],
-   "version": "1.0"
-}
diff --git a/chrome/test/data/extensions/browsertest/crash_25562/script.js b/chrome/test/data/extensions/browsertest/crash_25562/script.js
deleted file mode 100644
index b602eb8..0000000
--- a/chrome/test/data/extensions/browsertest/crash_25562/script.js
+++ /dev/null
@@ -1,7 +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.
-
-if (window == top) {
-  chrome.extension.connect().postMessage(true);
-}