Page actions should be able to change the popup on a per-tab basis.

This change is a prerequisite to change 545068.

BUG=27526
TEST=Added unit tests, manual testing on mac, linux, windows.

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

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

11 files changed, 143 insertions, 42 deletions

diff --git a/chrome/browser/cocoa/extensions/browser_actions_controller.mm b/chrome/browser/cocoa/extensions/browser_actions_controller.mm
index fda32b6..e17f12d 100644
--- a/chrome/browser/cocoa/extensions/browser_actions_controller.mm
+++ b/chrome/browser/cocoa/extensions/browser_actions_controller.mm
@@ -220,8 +220,14 @@ class ExtensionsServiceObserverBridge : public NotificationObserver {
 }
 
 - (void)browserActionClicked:(BrowserActionButton*)sender {
+  int tabId = [self currentTabId];
+  if (tabId < 0) {
+    NOTREACHED() << "No current tab.";
+    return;
+  }
+
   ExtensionAction* action = [sender extension]->browser_action();
-  if (action->has_popup() && !popupController_) {
+  if (action->HasPopup(tabId) && !popupController_) {
     NSString* extensionId = base::SysUTF8ToNSString([sender extension]->id());
     // If the extension ID is not valid UTF-8, then the NSString will be nil
     // and an exception will be thrown when calling objectForKey below, hosing
@@ -239,10 +245,12 @@ class ExtensionsServiceObserverBridge : public NotificationObserver {
     // Adjust the anchor point to be at the center of the browser action button.
     arrowPoint.x += kBrowserActionWidth / 2;
 
-    popupController_ = [ExtensionPopupController showURL:action->popup_url()
-                                               inBrowser:browser_
-                                              anchoredAt:arrowPoint
-                                           arrowLocation:kTopRight];
+    popupController_ = [ExtensionPopupController
+            showURL:action->GetPopupUrl(tabId)
+          inBrowser:browser_
+         anchoredAt:arrowPoint
+      arrowLocation:kTopRight];
+
     [[NSNotificationCenter defaultCenter]
         addObserver:self
            selector:@selector(popupWillClose:)
diff --git a/chrome/browser/cocoa/location_bar_view_mac.mm b/chrome/browser/cocoa/location_bar_view_mac.mm
index aa1b080..f206bda 100644
--- a/chrome/browser/cocoa/location_bar_view_mac.mm
+++ b/chrome/browser/cocoa/location_bar_view_mac.mm
@@ -543,7 +543,14 @@ LocationBarViewMac::PageActionImageView::~PageActionImageView() {
 // Overridden from LocationBarImageView. Either notify listeners or show a
 // popup depending on the Page Action.
 bool LocationBarViewMac::PageActionImageView::OnMousePressed(NSRect bounds) {
-  if (page_action_->has_popup()) {
+  if (current_tab_id_ < 0) {
+    NOTREACHED() << "No current tab.";
+    // We don't want other code to try and handle this click.  Returning true
+    // prevents this by indicating that we handled it.
+    return true;
+  }
+
+  if (page_action_->HasPopup(current_tab_id_)) {
     AutocompleteTextField* textField = owner_->GetAutocompleteTextField();
     NSWindow* window = [textField window];
     NSRect relativeBounds = [[window contentView] convertRect:bounds
@@ -555,16 +562,16 @@ bool LocationBarViewMac::PageActionImageView::OnMousePressed(NSRect bounds) {
     // Adjust the anchor point to be at the center of the page action icon.
     arrowPoint.x += [GetImage() size].width / 2;
 
-    popup_controller_ =
-        [ExtensionPopupController showURL:page_action_->popup_url()
-                                inBrowser:BrowserList::GetLastActive()
-                               anchoredAt:arrowPoint
-                            arrowLocation:kTopRight];
-    } else {
-      ExtensionBrowserEventRouter::GetInstance()->PageActionExecuted(
-          profile_, page_action_->extension_id(), page_action_->id(),
-          current_tab_id_, current_url_.spec(),
-          1);  // TODO(pamg): Add support for middle and right buttons.
+    popup_controller_ = [ExtensionPopupController
+            showURL:page_action_->GetPopupUrl(current_tab_id_)
+          inBrowser:BrowserList::GetLastActive()
+         anchoredAt:arrowPoint
+      arrowLocation:kTopRight];
+  } else {
+    ExtensionBrowserEventRouter::GetInstance()->PageActionExecuted(
+        profile_, page_action_->extension_id(), page_action_->id(),
+        current_tab_id_, current_url_.spec(),
+        1);
   }
   return true;
 }
diff --git a/chrome/browser/gtk/browser_actions_toolbar_gtk.cc b/chrome/browser/gtk/browser_actions_toolbar_gtk.cc
index 142898b..70034e8 100644
--- a/chrome/browser/gtk/browser_actions_toolbar_gtk.cc
+++ b/chrome/browser/gtk/browser_actions_toolbar_gtk.cc
@@ -188,9 +188,17 @@ class BrowserActionButton : public NotificationObserver,
   }
 
   static void OnClicked(GtkWidget* widget, BrowserActionButton* action) {
-    if (action->extension_->browser_action()->has_popup()) {
+    ExtensionAction* browser_action = action->extension_->browser_action();
+
+    int tab_id = action->toolbar_->GetCurrentTabId();
+    if (tab_id < 0) {
+      NOTREACHED() << "No current tab.";
+      return;
+    }
+
+    if (browser_action->HasPopup(tab_id)) {
       ExtensionPopupGtk::Show(
-          action->extension_->browser_action()->popup_url(),
+          action->extension_->browser_action()->GetPopupUrl(tab_id),
           action->toolbar_->browser(),
           gtk_util::GetWidgetRectRelativeToToplevel(widget));
     } else {
diff --git a/chrome/browser/gtk/location_bar_view_gtk.cc b/chrome/browser/gtk/location_bar_view_gtk.cc
index 451705d..2842fcd 100644
--- a/chrome/browser/gtk/location_bar_view_gtk.cc
+++ b/chrome/browser/gtk/location_bar_view_gtk.cc
@@ -1050,9 +1050,9 @@ gboolean LocationBarViewGtk::PageActionViewGtk::OnButtonPressed(
     GtkWidget* sender,
     GdkEvent* event) {
   if (event->button.button != 3) {
-    if (page_action_->has_popup()) {
+    if (page_action_->HasPopup(current_tab_id_)) {
       ExtensionPopupGtk::Show(
-          page_action_->popup_url(),
+          page_action_->GetPopupUrl(current_tab_id_),
           owner_->browser_,
           gtk_util::GetWidgetRectRelativeToToplevel(event_box_.get()));
     } else {
diff --git a/chrome/browser/views/browser_actions_container.cc b/chrome/browser/views/browser_actions_container.cc
index 02c213a..15bb3e7 100644
--- a/chrome/browser/views/browser_actions_container.cc
+++ b/chrome/browser/views/browser_actions_container.cc
@@ -158,7 +158,22 @@ void BrowserActionButton::Observe(NotificationType type,
 }
 
 bool BrowserActionButton::IsPopup() {
-  return browser_action_->has_popup();
+  int tab_id = panel_->GetCurrentTabId();
+  if (tab_id < 0) {
+    NOTREACHED() << "Button is not on a specific tab.";
+    return false;
+  }
+  return browser_action_->HasPopup(tab_id);
+}
+
+GURL BrowserActionButton::GetPopupUrl() {
+  int tab_id = panel_->GetCurrentTabId();
+  if (tab_id < 0) {
+    NOTREACHED() << "Button is not on a specific tab.";
+    GURL empty_url;
+    return empty_url;
+  }
+  return browser_action_->GetPopupUrl(tab_id);
 }
 
 bool BrowserActionButton::Activate() {
@@ -510,7 +525,8 @@ void BrowserActionsContainer::OnBrowserActionExecuted(
         toolbar_->browser()->window()->GetNativeHandle();
     BubbleBorder::ArrowLocation arrow_location = UILayoutIsRightToLeft() ?
         BubbleBorder::TOP_LEFT : BubbleBorder::TOP_RIGHT;
-    popup_ = ExtensionPopup::Show(browser_action->popup_url(),
+
+    popup_ = ExtensionPopup::Show(button->GetPopupUrl(),
                                   toolbar_->browser(),
                                   toolbar_->browser()->profile(),
                                   frame_window,
diff --git a/chrome/browser/views/browser_actions_container.h b/chrome/browser/views/browser_actions_container.h
index c668965..3f769b7 100644
--- a/chrome/browser/views/browser_actions_container.h
+++ b/chrome/browser/views/browser_actions_container.h
@@ -76,6 +76,7 @@ class BrowserActionButton : public views::MenuButton,
 
   // Does this button's action have a popup?
   virtual bool IsPopup();
+  virtual GURL GetPopupUrl();
 
   // Notifications when to set button state to pushed/not pushed (for when the
   // popup/context menu is hidden or shown by the container).
diff --git a/chrome/browser/views/location_bar_view.cc b/chrome/browser/views/location_bar_view.cc
index 03a17f9..130ff4c 100644
--- a/chrome/browser/views/location_bar_view.cc
+++ b/chrome/browser/views/location_bar_view.cc
@@ -1316,7 +1316,12 @@ LocationBarView::PageActionImageView::~PageActionImageView() {
 }
 
 void LocationBarView::PageActionImageView::ExecuteAction(int button) {
-  if (page_action_->has_popup()) {
+  if (current_tab_id_ < 0) {
+    NOTREACHED() << "No current tab.";
+    return;
+  }
+
+  if (page_action_->HasPopup(current_tab_id_)) {
     // In tests, GetLastActive could return NULL, so we need to have
     // a fallback.
     // TODO(erikkay): Find a better way to get the Browser that this
@@ -1342,7 +1347,7 @@ void LocationBarView::PageActionImageView::ExecuteAction(int button) {
     rect.set_x(origin.x());
     rect.set_y(origin.y());
 
-    popup_ = ExtensionPopup::Show(page_action_->popup_url(),
+    popup_ = ExtensionPopup::Show(page_action_->GetPopupUrl(current_tab_id_),
                                   browser,
                                   browser->profile(),
                                   browser->window()->GetNativeHandle(),
diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc
index 1ad3ee2..f6de4ab 100644
--- a/chrome/common/extensions/extension.cc
+++ b/chrome/common/extensions/extension.cc
@@ -457,7 +457,7 @@ ExtensionAction* Extension::LoadExtensionActionHelper(
           errors::kInvalidPageActionPopupPath, url_str);
       return NULL;
     }
-    result->set_popup_url(url);
+    result->SetPopupUrl(ExtensionAction::kDefaultTabId, url);
   } else if (!url_str.empty()) {
     GURL url = GetResourceURL(url_str);
     if (!url.is_valid()) {
@@ -465,7 +465,7 @@ ExtensionAction* Extension::LoadExtensionActionHelper(
           errors::kInvalidPageActionPopupPath, url_str);
       return NULL;
     }
-    result->set_popup_url(url);
+    result->SetPopupUrl(ExtensionAction::kDefaultTabId, url);
   }
 
   return result.release();
diff --git a/chrome/common/extensions/extension_action.cc b/chrome/common/extensions/extension_action.cc
index d8d214c..cb05fa5 100755
--- a/chrome/common/extensions/extension_action.cc
+++ b/chrome/common/extensions/extension_action.cc
@@ -92,6 +92,7 @@ void ExtensionAction::ClearAllValuesForTab(int tab_id) {
   badge_text_color_.erase(tab_id);
   badge_background_color_.erase(tab_id);
   visible_.erase(tab_id);
+  popup_url_.erase(tab_id);
 }
 
 void ExtensionAction::PaintBadge(gfx::Canvas* canvas,
diff --git a/chrome/common/extensions/extension_action.h b/chrome/common/extensions/extension_action.h
index 59ceb57..9fbbff4 100755
--- a/chrome/common/extensions/extension_action.h
+++ b/chrome/common/extensions/extension_action.h
@@ -36,11 +36,6 @@ class ExtensionAction {
     extension_id_ = extension_id;
   }
 
-  // popup details
-  const GURL& popup_url() const { return popup_url_; }
-  void set_popup_url(const GURL& url) { popup_url_ = url; }
-  bool has_popup() const { return !popup_url_.is_empty(); }
-
   // action id -- only used with legacy page actions API
   std::string id() const { return id_; }
   void set_id(const std::string& id) { id_ = id; }
@@ -48,10 +43,32 @@ class ExtensionAction {
   // static icon paths from manifest -- only used with legacy page actions API.
   std::vector<std::string>* icon_paths() { return &icon_paths_; }
 
-  // title
+  // Set the url which the popup will load when the user clicks this action's
+  // icon.  Setting an empty URL will disable the popup for a given tab.
+  void SetPopupUrl(int tab_id, const GURL& url) {
+    // We store |url| even if it is empty, rather than removing a URL from the
+    // map.  If an extension has a default popup, and removes it for a tab via
+    // the API, we must remember that there is no popup for that specific tab.
+    // If we removed the tab's URL, GetPopupURL would incorrectly return the
+    // default URL.
+    SetValue(&popup_url_, tab_id, url);
+  }
+
+  // Use HasPopup() to see if a popup should be displayed.
+  bool HasPopup(int tab_id) {
+    return !GetPopupUrl(tab_id).is_empty();
+  }
+
+  // Get the URL to display in a popup.
+  GURL GetPopupUrl(int tab_id) { return GetValue(&popup_url_, tab_id); }
+
+  // Set this action's title on a specific tab.
   void SetTitle(int tab_id, const std::string& title) {
     SetValue(&title_, tab_id, title);
   }
+
+  // If tab |tab_id| has a set title, return it.  Otherwise, return
+  // the default title.
   std::string GetTitle(int tab_id) { return GetValue(&title_, tab_id); }
 
   // Icons are a bit different because the default value can be set to either a
@@ -61,13 +78,15 @@ class ExtensionAction {
   // To get the default icon, first check for the bitmap. If it is null, check
   // for the path.
 
-  // Icon bitmap.
+  // Set this action's icon bitmap on a specific tab.
   void SetIcon(int tab_id, const SkBitmap& bitmap) {
     SetValue(&icon_, tab_id, bitmap);
   }
+  // Get the icon for a tab, or the default if no icon was set.
   SkBitmap GetIcon(int tab_id) { return GetValue(&icon_, tab_id); }
 
-  // Icon index -- for use with icon_paths(), only used in page actions.
+  // Set this action's icon index for a specific tab.  For use with
+  // icon_paths(), only used in page actions.
   void SetIconIndex(int tab_id, int index) {
     if (static_cast<size_t>(index) >= icon_paths_.size()) {
       NOTREACHED();
@@ -75,6 +94,8 @@ class ExtensionAction {
     }
     SetValue(&icon_index_, tab_id, index);
   }
+  // Get this action's icon index for a tab, or the default if no icon index
+  // was set.
   int GetIconIndex(int tab_id) {
     return GetValue(&icon_index_, tab_id);
   }
@@ -88,32 +109,41 @@ class ExtensionAction {
     return default_icon_path_;
   }
 
-  // badge text
+  // Set this action's badge text on a specific tab.
   void SetBadgeText(int tab_id, const std::string& text) {
     SetValue(&badge_text_, tab_id, text);
   }
-  std::string GetBadgeText(int tab_id) { return GetValue(&badge_text_, tab_id); }
+  // Get the badge text for a tab, or the default if no badge text was set.
+  std::string GetBadgeText(int tab_id) {
+    return GetValue(&badge_text_, tab_id);
+  }
 
-  // badge text color
+  // Set this action's badge text color on a specific tab.
   void SetBadgeTextColor(int tab_id, const SkColor& text_color) {
     SetValue(&badge_text_color_, tab_id, text_color);
   }
+  // Get the text color for a tab, or the default color if no text color
+  // was set.
   SkColor GetBadgeTextColor(int tab_id) {
     return GetValue(&badge_text_color_, tab_id);
   }
 
-  // badge background color
+  // Set this action's badge background color on a specific tab.
   void SetBadgeBackgroundColor(int tab_id, const SkColor& color) {
     SetValue(&badge_background_color_, tab_id, color);
   }
+  // Get the badge backround color for a tab, or the default if no color
+  // was set.
   SkColor GetBadgeBackgroundColor(int tab_id) {
     return GetValue(&badge_background_color_, tab_id);
   }
 
-  // visibility
+  // Set this action's badge visibility on a specific tab.
   void SetIsVisible(int tab_id, bool value) {
     SetValue(&visible_, tab_id, value);
   }
+  // Get the badge visibility for a tab, or the default badge visibility
+  // if none was set.
   bool GetIsVisible(int tab_id) {
     return GetValue(&visible_, tab_id);
   }
@@ -154,6 +184,7 @@ class ExtensionAction {
 
   // Each of these data items can have both a global state (stored with the key
   // kDefaultTabId), or tab-specific state (stored with the tab_id as the key).
+  std::map<int, GURL> popup_url_;
   std::map<int, std::string> title_;
   std::map<int, SkBitmap> icon_;
   std::map<int, int> icon_index_;  // index into icon_paths_
@@ -164,9 +195,6 @@ class ExtensionAction {
 
   std::string default_icon_path_;
 
-  // If the action has a popup, it has a URL and a height.
-  GURL popup_url_;
-
   // 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/common/extensions/extension_action_unittest.cc b/chrome/common/extensions/extension_action_unittest.cc
index f3a9c90..2c5c762 100644
--- a/chrome/common/extensions/extension_action_unittest.cc
+++ b/chrome/common/extensions/extension_action_unittest.cc
@@ -138,4 +138,31 @@ TEST(ExtensionActionTest, TabSpecificState) {
   ASSERT_EQ(0xFF0000FFu, action.GetBadgeBackgroundColor(1));
   action.ClearAllValuesForTab(100);
   ASSERT_EQ(0xFF0000FFu, action.GetBadgeBackgroundColor(100));
+
+  // popup url
+  GURL url_unset;
+  GURL url_foo("http://www.example.com/foo.html");
+  GURL url_bar("http://www.example.com/bar.html");
+  GURL url_baz("http://www.example.com/baz.html");
+
+  ASSERT_EQ(url_unset, action.GetPopupUrl(1));
+  ASSERT_EQ(url_unset, action.GetPopupUrl(100));
+  ASSERT_FALSE(action.HasPopup(1));
+  ASSERT_FALSE(action.HasPopup(100));
+
+  action.SetPopupUrl(ExtensionAction::kDefaultTabId, url_foo);
+  ASSERT_EQ(url_foo, action.GetPopupUrl(1));
+  ASSERT_EQ(url_foo, action.GetPopupUrl(100));
+
+  action.SetPopupUrl(100, url_bar);
+  ASSERT_EQ(url_foo, action.GetPopupUrl(1));
+  ASSERT_EQ(url_bar, action.GetPopupUrl(100));
+
+  action.SetPopupUrl(ExtensionAction::kDefaultTabId, url_baz);
+  ASSERT_EQ(url_baz, action.GetPopupUrl(1));
+  ASSERT_EQ(url_bar, action.GetPopupUrl(100));
+
+  action.ClearAllValuesForTab(100);
+  ASSERT_EQ(url_baz, action.GetPopupUrl(1));
+  ASSERT_EQ(url_baz, action.GetPopupUrl(100));
 }