Force page-action views to update after reloading an extension, by deleting

them all. Otherwise, since the view count is unchanged, the views continue to
use stale extension information and the page-action icon fails to be displayed.

Fix excessive timeout logging in WaitForPageActionVisibilityChangeTo().

BUG=http://crbug.com/21324
TEST=write page-action extension, load as unpacked, reload, verify that icon
  is shown on a matching page. Also covered by browser_tests unit test.

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

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

13 files changed, 102 insertions, 13 deletions

diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index c8c1da1..e0b5336 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -2142,6 +2142,8 @@ void Browser::Observe(NotificationType type,
     }
 
     case NotificationType::EXTENSION_UNLOADED: {
+      window()->GetLocationBar()->InvalidatePageActions();
+
       // Close any tabs from the unloaded extension.
       Extension* extension = Details<Extension>(details).ptr();
       for (int i = 0; i < tabstrip_model_.count(); i++) {
@@ -2156,6 +2158,8 @@ void Browser::Observe(NotificationType type,
     }
 
     case NotificationType::EXTENSION_PROCESS_CRASHED: {
+      window()->GetLocationBar()->InvalidatePageActions();
+
       TabContents* tab_contents = GetSelectedTabContents();
       if (!tab_contents)
         break;
diff --git a/chrome/browser/cocoa/location_bar_view_mac.h b/chrome/browser/cocoa/location_bar_view_mac.h
index e35b421..f04b11e 100644
--- a/chrome/browser/cocoa/location_bar_view_mac.h
+++ b/chrome/browser/cocoa/location_bar_view_mac.h
@@ -44,6 +44,7 @@ class LocationBarViewMac : public AutocompleteEditController,
   virtual void FocusLocation();
   virtual void FocusSearch();
   virtual void UpdatePageActions() { /* http://crbug.com/12281 */ }
+  virtual void InvalidatePageActions() { /* TODO(port): implement this */ }
   virtual void SaveStateToContents(TabContents* contents);
   virtual void Revert();
   virtual AutocompleteEditView* location_entry() {
@@ -52,6 +53,7 @@ class LocationBarViewMac : public AutocompleteEditController,
   virtual LocationBarTesting* GetLocationBarForTesting() { return this; }
 
   // Overriden from LocationBarTesting:
+  virtual int PageActionCount();
   virtual int PageActionVisibleCount();
 
   // Updates the location bar.  Resets the bar's permanent text and
diff --git a/chrome/browser/cocoa/location_bar_view_mac.mm b/chrome/browser/cocoa/location_bar_view_mac.mm
index 6410984..bbf8a66 100644
--- a/chrome/browser/cocoa/location_bar_view_mac.mm
+++ b/chrome/browser/cocoa/location_bar_view_mac.mm
@@ -241,6 +241,11 @@ void LocationBarViewMac::Revert() {
   edit_view_->RevertAll();
 }
 
+int LocationBarViewMac::PageActionCount() {
+  NOTIMPLEMENTED();
+  return -1;
+}
+
 int LocationBarViewMac::PageActionVisibleCount() {
   NOTIMPLEMENTED();
   return -1;
diff --git a/chrome/browser/extensions/extension_browsertest.cc b/chrome/browser/extensions/extension_browsertest.cc
index 9c14779..0bb5542 100644
--- a/chrome/browser/extensions/extension_browsertest.cc
+++ b/chrome/browser/extensions/extension_browsertest.cc
@@ -115,13 +115,40 @@ bool ExtensionBrowserTest::InstallOrUpdateExtension(
   return WaitForExtensionHostsToLoad();
 }
 
+void ExtensionBrowserTest::UnloadExtension(const std::string& extension_id) {
+  ExtensionsService* service = browser()->profile()->GetExtensionsService();
+  service->UnloadExtension(extension_id);
+}
+
 void ExtensionBrowserTest::UninstallExtension(const std::string& extension_id) {
   ExtensionsService* service = browser()->profile()->GetExtensionsService();
   service->UninstallExtension(extension_id, false);
 }
 
-bool ExtensionBrowserTest::WaitForPageActionVisibilityChangeTo(
-    int count) {
+bool ExtensionBrowserTest::WaitForPageActionCountChangeTo(int count) {
+  base::Time start_time = base::Time::Now();
+  while (true) {
+    LocationBarTesting* loc_bar =
+        browser()->window()->GetLocationBar()->GetLocationBarForTesting();
+
+    int actions = loc_bar->PageActionCount();
+    if (actions == count)
+      return true;
+
+    if ((base::Time::Now() - start_time).InMilliseconds() > kTimeoutMs) {
+      std::cout << "Timed out waiting for page actions to (un)load.\n"
+                << "Currently loaded page actions: " << IntToString(actions)
+                << "\n";
+      return false;
+    }
+
+    MessageLoop::current()->PostDelayedTask(FROM_HERE,
+                                            new MessageLoop::QuitTask, 200);
+    ui_test_utils::RunMessageLoop();
+  }
+}
+
+bool ExtensionBrowserTest::WaitForPageActionVisibilityChangeTo(int count) {
   base::Time start_time = base::Time::Now();
   while (true) {
     LocationBarTesting* loc_bar =
@@ -131,11 +158,12 @@ bool ExtensionBrowserTest::WaitForPageActionVisibilityChangeTo(
     if (visible == count)
       return true;
 
-    if ((base::Time::Now() - start_time).InMilliseconds() > kTimeoutMs)
+    if ((base::Time::Now() - start_time).InMilliseconds() > kTimeoutMs) {
+      std::cout << "Timed out waiting for page actions to become (in)visible.\n"
+                << "Currently visible page actions: " << IntToString(visible)
+                << "\n";
       return false;
-
-    std::cout << "Timed out waiting for page actions to become visible."
-              << "Currently visible page actions: " << IntToString(visible);
+    }
 
     MessageLoop::current()->PostDelayedTask(FROM_HERE,
                                             new MessageLoop::QuitTask, 200);
diff --git a/chrome/browser/extensions/extension_browsertest.h b/chrome/browser/extensions/extension_browsertest.h
index 1910229..943bdd4 100644
--- a/chrome/browser/extensions/extension_browsertest.h
+++ b/chrome/browser/extensions/extension_browsertest.h
@@ -38,8 +38,13 @@ class ExtensionBrowserTest
     return InstallOrUpdateExtension(id, path, expected_change);
   }
 
+  void UnloadExtension(const std::string& extension_id);
+
   void UninstallExtension(const std::string& extension_id);
 
+  // Wait for the total number of page actions to change to |count|.
+  bool WaitForPageActionCountChangeTo(int count);
+
   // Wait for the number of visible page actions to change to |count|.
   bool WaitForPageActionVisibilityChangeTo(int count);
 
diff --git a/chrome/browser/extensions/extension_browsertests_misc.cc b/chrome/browser/extensions/extension_browsertests_misc.cc
index 059016c..490ef9c 100644
--- a/chrome/browser/extensions/extension_browsertests_misc.cc
+++ b/chrome/browser/extensions/extension_browsertests_misc.cc
@@ -27,6 +27,9 @@
 #include "chrome/test/ui_test_utils.h"
 #include "net/base/net_util.h"
 
+// ID assigned to the first unpacked extension loaded by LoadExtension().
+#define kDefaultExtensionID "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+
 // Looks for an ExtensionHost whose URL has the given path component (including
 // leading slash).  Also verifies that the expected number of hosts are loaded.
 static ExtensionHost* FindHostWithPath(ExtensionProcessManager* manager,
@@ -220,6 +223,27 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, PageAction) {
   ui_test_utils::NavigateToURL(browser(), net::FilePathToFileURL(no_feed));
   ASSERT_TRUE(WaitForPageActionVisibilityChangeTo(0));
 }
+
+// Tests that the location bar forgets about unloaded page actions.
+IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, UnloadPageAction) {
+  FilePath extension_path(test_data_dir_.AppendASCII("samples")
+                                        .AppendASCII("subscribe_page_action"));
+  ASSERT_TRUE(LoadExtension(extension_path));
+
+  // Navigation prompts the location bar to load page actions.
+  FilePath test_dir;
+  PathService::Get(chrome::DIR_TEST_DATA, &test_dir);
+  FilePath feed = test_dir.AppendASCII("feeds")
+                          .AppendASCII("feed.html");
+
+  ui_test_utils::NavigateToURL(browser(), net::FilePathToFileURL(feed));
+  ASSERT_TRUE(WaitForPageActionCountChangeTo(1));
+
+  UnloadExtension(kDefaultExtensionID);
+
+  // Make sure the page action goes away when it's unloaded.
+  ASSERT_TRUE(WaitForPageActionCountChangeTo(0));
+}
 #endif  // defined(OS_WIN) || defined(OS_LINUX)
 
 GURL GetFeedUrl(const std::string& feed_page) {
@@ -562,7 +586,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, WindowOpenExtension) {
 
   TabContents* newtab = WindowOpenHelper(
       browser(),
-      GURL("chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/test.html"),
+      GURL("chrome-extension://" kDefaultExtensionID "/test.html"),
       "newtab.html");
 
   bool result = false;
@@ -579,8 +603,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, WindowOpenInvalidExtension) {
 
   WindowOpenHelper(
       browser(),
-      GURL("chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/test.html"),
-      "chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab/newtab.html");
+      GURL("chrome-extension://" kDefaultExtensionID "/test.html"),
+      "chrome-extension://thisissurelynotavalidextensionid/newtab.html");
 
   // If we got to this point, we didn't crash, so we're good.
 }
@@ -595,7 +619,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, WindowOpenNoPrivileges) {
   TabContents* newtab = WindowOpenHelper(
       browser(),
       GURL("about:blank"),
-      "chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/newtab.html");
+      "chrome-extension://" kDefaultExtensionID "/newtab.html");
 
   // Extension API should fail.
   bool result = false;
diff --git a/chrome/browser/extensions/extensions_service.h b/chrome/browser/extensions/extensions_service.h
index 85822a1..153c723 100644
--- a/chrome/browser/extensions/extensions_service.h
+++ b/chrome/browser/extensions/extensions_service.h
@@ -150,7 +150,8 @@ class ExtensionsService
   // Unload the specified extension.
   void UnloadExtension(const std::string& extension_id);
 
-  // Unload all extensions.
+  // Unload all extensions. This is currently only called on shutdown, and
+  // does not send notifications.
   void UnloadAllExtensions();
 
   // Called only by testing.
diff --git a/chrome/browser/gtk/location_bar_view_gtk.cc b/chrome/browser/gtk/location_bar_view_gtk.cc
index 3184cd4..aaf2fe4 100644
--- a/chrome/browser/gtk/location_bar_view_gtk.cc
+++ b/chrome/browser/gtk/location_bar_view_gtk.cc
@@ -421,6 +421,10 @@ void LocationBarViewGtk::UpdatePageActions() {
     gtk_widget_hide(page_action_hbox_);
 }
 
+void LocationBarViewGtk::InvalidatePageActions() {
+  page_action_views_.reset();
+}
+
 void LocationBarViewGtk::SaveStateToContents(TabContents* contents) {
   location_entry_->SaveStateToTab(contents);
 }
diff --git a/chrome/browser/gtk/location_bar_view_gtk.h b/chrome/browser/gtk/location_bar_view_gtk.h
index 22bf7d1..3605027 100644
--- a/chrome/browser/gtk/location_bar_view_gtk.h
+++ b/chrome/browser/gtk/location_bar_view_gtk.h
@@ -78,6 +78,7 @@ class LocationBarViewGtk : public AutocompleteEditController,
   virtual void FocusLocation();
   virtual void FocusSearch();
   virtual void UpdatePageActions();
+  virtual void InvalidatePageActions();
   virtual void SaveStateToContents(TabContents* contents);
   virtual void Revert();
   virtual AutocompleteEditView* location_entry() {
@@ -86,6 +87,7 @@ class LocationBarViewGtk : public AutocompleteEditController,
   virtual LocationBarTesting* GetLocationBarForTesting() { return this; }
 
   // Implement the LocationBarTesting interface.
+  virtual int PageActionCount() { return page_action_views_.size(); }
   virtual int PageActionVisibleCount();
 
   // Implement the NotificationObserver interface.
diff --git a/chrome/browser/location_bar.h b/chrome/browser/location_bar.h
index c8e9293..6cf544d 100644
--- a/chrome/browser/location_bar.h
+++ b/chrome/browser/location_bar.h
@@ -39,7 +39,7 @@ class LocationBar {
   // Accepts the current string of text entered in the location bar.
   virtual void AcceptInput() = 0;
 
-  // Accept the current input, overriding the disposition.
+  // Accepts the current input, overriding the disposition.
   virtual void AcceptInputWithDisposition(WindowOpenDisposition) = 0;
 
   // Focuses and selects the contents of the location bar.
@@ -49,9 +49,13 @@ class LocationBar {
   // focus to it.
   virtual void FocusSearch() = 0;
 
-  // Update the state of the page actions.
+  // Updates the state of the page actions.
   virtual void UpdatePageActions() = 0;
 
+  // Called when the page-action data needs to be refreshed, e.g. when an
+  // extension is unloaded or crashes.
+  virtual void InvalidatePageActions() = 0;
+
   // Saves the state of the location bar to the specified TabContents, so that
   // it can be restored later. (Done when switching tabs).
   virtual void SaveStateToContents(TabContents* contents) = 0;
@@ -68,6 +72,9 @@ class LocationBar {
 
 class LocationBarTesting {
  public:
+  // Returns the total number of page actions in the Omnibox.
+  virtual int PageActionCount() = 0;
+
   // Returns the number of visible page actions in the Omnibox.
   virtual int PageActionVisibleCount() = 0;
 };
diff --git a/chrome/browser/views/location_bar_view.cc b/chrome/browser/views/location_bar_view.cc
index dcaa2d1..310ba52 100644
--- a/chrome/browser/views/location_bar_view.cc
+++ b/chrome/browser/views/location_bar_view.cc
@@ -284,6 +284,10 @@ void LocationBarView::UpdatePageActions() {
   SchedulePaint();
 }
 
+void LocationBarView::InvalidatePageActions() {
+  DeletePageActionViews();
+}
+
 void LocationBarView::Focus() {
 #if defined(OS_WIN)
   ::SetFocus(location_entry_->m_hWnd);
diff --git a/chrome/browser/views/location_bar_view.h b/chrome/browser/views/location_bar_view.h
index 3fdfcdc..a9e0a54 100644
--- a/chrome/browser/views/location_bar_view.h
+++ b/chrome/browser/views/location_bar_view.h
@@ -145,6 +145,7 @@ class LocationBarView : public LocationBar,
   virtual void FocusLocation();
   virtual void FocusSearch();
   virtual void UpdatePageActions();
+  virtual void InvalidatePageActions();
   virtual void SaveStateToContents(TabContents* contents);
   virtual void Revert();
   virtual AutocompleteEditView* location_entry() {
@@ -153,6 +154,7 @@ class LocationBarView : public LocationBar,
   virtual LocationBarTesting* GetLocationBarForTesting() { return this; }
 
   // Overridden from LocationBarTesting:
+  virtual int PageActionCount() { return page_action_image_views_.size(); }
   virtual int PageActionVisibleCount();
 
   static const int kVertMargin;
diff --git a/chrome/test/test_location_bar.h b/chrome/test/test_location_bar.h
index 995d07a..cd96629 100644
--- a/chrome/test/test_location_bar.h
+++ b/chrome/test/test_location_bar.h
@@ -38,6 +38,7 @@ class TestLocationBar : public LocationBar {
   virtual void FocusLocation() {}
   virtual void FocusSearch() {}
   virtual void UpdatePageActions() {}
+  virtual void InvalidatePageActions() {}
   virtual void SaveStateToContents(TabContents* contents) {}
   virtual void Revert() {}
   virtual AutocompleteEditView* location_entry() {