Only return the visible page actions from PageActionController.

chrome/browser/ui/gtk/location_bar_view_gtk.cc

Index: chrome/browser/ui/gtk/location_bar_view_gtk.cc
diff --git a/chrome/browser/ui/gtk/location_bar_view_gtk.cc b/chrome/browser/ui/gtk/location_bar_view_gtk.cc
index 36727409d5c8fd35a09c4ff896922a843b9ac746..4dc2da1a0cec82ebe9276b2ff8890190fd5638bc 100644
--- a/chrome/browser/ui/gtk/location_bar_view_gtk.cc
+++ b/chrome/browser/ui/gtk/location_bar_view_gtk.cc
@@ -26,7 +26,6 @@
 #include "chrome/browser/defaults.h"
 #include "chrome/browser/extensions/api/commands/extension_command_service.h"
 #include "chrome/browser/extensions/api/commands/extension_command_service_factory.h"
-#include "chrome/browser/extensions/action_box_controller.h"
 #include "chrome/browser/extensions/extension_browser_event_router.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
@@ -356,6 +355,10 @@ void LocationBarViewGtk::Init(bool popup_window_mode) {
   registrar_.Add(this,
                  chrome::NOTIFICATION_BROWSER_THEME_CHANGED,
                  content::Source<ThemeService>(theme_service_));
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_EXTENSION_ACTION_BOX_UPDATED,
+                 content::Source<Profile>(browser()->profile()));
+
   edit_bookmarks_enabled_.Init(prefs::kEditBookmarksEnabled,
                                profile->GetPrefs(), this);
 
@@ -444,15 +447,12 @@ void LocationBarViewGtk::SetPreviewEnabledPageAction(
     ExtensionAction *page_action,
     bool preview_enabled) {
   DCHECK(page_action);
-  UpdatePageActions();
-  for (ScopedVector<PageActionViewGtk>::iterator iter =
-       page_action_views_.begin(); iter != page_action_views_.end();
-       ++iter) {
-    if ((*iter)->page_action() == page_action) {
-      (*iter)->set_preview_enabled(preview_enabled);
-      UpdatePageActions();
-      return;
-    }
+  if (preview_enabled && !preview_enabled_actions_.count(page_action)) {
+    preview_enabled_actions_.insert(page_action);

[Matt Perry, 2012/05/17 00:17:24]

std::set allows you to test and set simultaneously. You could restructure this
as

if (preview_enabled && insert(page_action).second)
  update();
else if (!preview_enabled && erase(page_action) > 0)
  update();

[not at google - send to devlin, 2012/05/17 01:30:35]

Done.

+    UpdatePageActions();
+  } else if (!preview_enabled && preview_enabled_actions_.count(page_action)) {
+    preview_enabled_actions_.erase(page_action);
+    UpdatePageActions();
   }
 }
 
@@ -685,14 +685,19 @@ void LocationBarViewGtk::UpdateContentSettingsIcons() {
 }
 
 void LocationBarViewGtk::UpdatePageActions() {
-  ActionBoxController::DataList page_actions;
+  std::vector<ExtensionAction*> page_actions;
 
   TabContentsWrapper* tab_contents = GetTabContentsWrapper();
   if (tab_contents) {
     page_actions.swap(
-        *tab_contents->extension_action_box_controller()->GetAllBadgeData());
+        *tab_contents->extension_action_box_controller()->GetCurrentActions());
   }
 
+  // Add page actions for any extensions which have "preview enabled" and not
+  // already visible.
+  ActionBoxController::AddMissingActions(
+      preview_enabled_actions_, &page_actions);
+
   // 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()) {
@@ -700,7 +705,7 @@ void LocationBarViewGtk::UpdatePageActions() {
 
     for (size_t i = 0; i < page_actions.size(); ++i) {
       page_action_views_.push_back(
-          new PageActionViewGtk(this, page_actions[i].action));
+          new PageActionViewGtk(this, page_actions[i]));
       gtk_box_pack_end(GTK_BOX(page_action_hbox_.get()),
                        page_action_views_[i]->widget(), FALSE, FALSE, 0);
     }
@@ -715,7 +720,7 @@ void LocationBarViewGtk::UpdatePageActions() {
     GURL url = browser()->GetSelectedWebContents()->GetURL();
 
     for (size_t i = 0; i < page_action_views_.size(); i++) {
-      page_action_views_[i]->UpdateVisibility(
+      page_action_views_[i]->Update(
           toolbar_model_->input_in_progress() ? NULL : contents, url);
     }
   }
@@ -778,16 +783,7 @@ ExtensionAction* LocationBarViewGtk::GetPageAction(size_t index) {
 }
 
 ExtensionAction* LocationBarViewGtk::GetVisiblePageAction(size_t index) {
-  size_t visible_index = 0;
-  for (size_t i = 0; i < page_action_views_.size(); ++i) {
-    if (page_action_views_[i]->IsVisible()) {
-      if (index == visible_index++)
-        return page_action_views_[i]->page_action();
-    }
-  }
-
-  NOTREACHED();
-  return NULL;
+  return page_action_views_[index]->page_action();
 }
 
 void LocationBarViewGtk::TestPageActionPressed(size_t index) {
@@ -808,6 +804,15 @@ void LocationBarViewGtk::Observe(int type,
     return;
   }
 
+  if (type == chrome::NOTIFICATION_EXTENSION_ACTION_BOX_UPDATED) {
+    // Only update if the updated action box was for the active tab contents.
+    TabContentsWrapper* target_tab =
+        content::Details<TabContentsWrapper>(details).ptr();
+    if (target_tab == GetTabContentsWrapper())
+        UpdatePageActions();

[Evan Stade, 2012/05/16 21:58:17]

too much indent

[not at google - send to devlin, 2012/05/17 01:30:35]

Done.

+    return;
+  }
+
   DCHECK_EQ(type, chrome::NOTIFICATION_BROWSER_THEME_CHANGED);
 
   if (theme_service_->UsingNativeTheme()) {
@@ -1503,8 +1508,7 @@ LocationBarViewGtk::PageActionViewGtk::PageActionViewGtk(
       tracker_(this),
       current_tab_id_(-1),
       window_(NULL),
-      accel_group_(NULL),
-      preview_enabled_(false) {
+      accel_group_(NULL) {
   event_box_.Own(gtk_event_box_new());
   gtk_widget_set_size_request(event_box_.get(),
                               Extension::kPageActionIconMaxSize,
@@ -1521,6 +1525,7 @@ LocationBarViewGtk::PageActionViewGtk::PageActionViewGtk(
 
   image_.Own(gtk_image_new());
   gtk_container_add(GTK_CONTAINER(event_box_.get()), image_.get());
+  gtk_widget_show_all(event_box_.get());
 
   const Extension* extension = owner->browser()->profile()->
       GetExtensionService()->GetExtensionById(page_action->extension_id(),
@@ -1559,75 +1564,54 @@ LocationBarViewGtk::PageActionViewGtk::~PageActionViewGtk() {
     g_object_unref(last_icon_pixbuf_);
 }
 
-bool LocationBarViewGtk::PageActionViewGtk::IsVisible() {
-  return gtk_widget_get_visible(widget());
-}
-
-void LocationBarViewGtk::PageActionViewGtk::UpdateVisibility(
+void LocationBarViewGtk::PageActionViewGtk::Update(
     WebContents* contents, const GURL& url) {
   // Save this off so we can pass it back to the extension when the action gets
   // executed. See PageActionImageView::OnMousePressed.
   current_tab_id_ = contents ? ExtensionTabUtil::GetTabId(contents) : -1;
   current_url_ = url;
 
-  bool visible = contents &&
-      (preview_enabled_ || page_action_->GetIsVisible(current_tab_id_));
-  if (visible) {
-    // Set the tooltip.
-    gtk_widget_set_tooltip_text(event_box_.get(),
-        page_action_->GetTitle(current_tab_id_).c_str());
-
-    // Set the image.
-    // It can come from three places. In descending order of priority:
-    // - The developer can set it dynamically by path or bitmap. It will be in
-    //   page_action_->GetIcon().
-    // - The developer can set it dyanmically by index. It will be in
-    //   page_action_->GetIconIndex().
-    // - It can be set in the manifest by path. It will be in page_action_->
-    //   default_icon_path().
-
-    // First look for a dynamically set bitmap.
-    SkBitmap icon = page_action_->GetIcon(current_tab_id_);
-    GdkPixbuf* pixbuf = NULL;
-    if (!icon.isNull()) {
-      if (icon.pixelRef() != last_icon_skbitmap_.pixelRef()) {
-        if (last_icon_pixbuf_)
-          g_object_unref(last_icon_pixbuf_);
-        last_icon_skbitmap_ = icon;
-        last_icon_pixbuf_ = gfx::GdkPixbufFromSkBitmap(&icon);
-      }
-      DCHECK(last_icon_pixbuf_);
-      pixbuf = last_icon_pixbuf_;
-    } else {
-      // Otherwise look for a dynamically set index, or fall back to the
-      // default path.
-      int icon_index = page_action_->GetIconIndex(current_tab_id_);
-      std::string icon_path = (icon_index < 0) ?
-          page_action_->default_icon_path() :
-          page_action_->icon_paths()->at(icon_index);
-      if (!icon_path.empty()) {
-        PixbufMap::iterator iter = pixbufs_.find(icon_path);
-        if (iter != pixbufs_.end())
-          pixbuf = iter->second;
-      }
+  // Set the tooltip.
+  gtk_widget_set_tooltip_text(event_box_.get(),
+      page_action_->GetTitle(current_tab_id_).c_str());
+
+  // Set the image.
+  // It can come from three places. In descending order of priority:
+  // - The developer can set it dynamically by path or bitmap. It will be in
+  //   page_action_->GetIcon().
+  // - The developer can set it dyanmically by index. It will be in
+  //   page_action_->GetIconIndex().
+  // - It can be set in the manifest by path. It will be in page_action_->
+  //   default_icon_path().
+
+  // First look for a dynamically set bitmap.
+  SkBitmap icon = page_action_->GetIcon(current_tab_id_);
+  GdkPixbuf* pixbuf = NULL;
+  if (!icon.isNull()) {
+    if (icon.pixelRef() != last_icon_skbitmap_.pixelRef()) {
+      if (last_icon_pixbuf_)
+        g_object_unref(last_icon_pixbuf_);
+      last_icon_skbitmap_ = icon;
+      last_icon_pixbuf_ = gfx::GdkPixbufFromSkBitmap(&icon);
+    }
+    DCHECK(last_icon_pixbuf_);
+    pixbuf = last_icon_pixbuf_;
+  } else {
+    // Otherwise look for a dynamically set index, or fall back to the
+    // default path.
+    int icon_index = page_action_->GetIconIndex(current_tab_id_);
+    std::string icon_path = (icon_index < 0) ?

[Evan Stade, 2012/05/16 21:58:17]

no parens

[not at google - send to devlin, 2012/05/17 01:30:35]

Done.

+        page_action_->default_icon_path() :
+        page_action_->icon_paths()->at(icon_index);
+    if (!icon_path.empty()) {
+      PixbufMap::iterator iter = pixbufs_.find(icon_path);
+      if (iter != pixbufs_.end())
+        pixbuf = iter->second;
     }
-    // The pixbuf might not be loaded yet.
-    if (pixbuf)
-      gtk_image_set_from_pixbuf(GTK_IMAGE(image_.get()), pixbuf);
-  }
-
-  bool old_visible = IsVisible();
-  if (visible)
-    gtk_widget_show_all(event_box_.get());
-  else
-    gtk_widget_hide_all(event_box_.get());
-
-  if (visible != old_visible) {
-    content::NotificationService::current()->Notify(
-        chrome::NOTIFICATION_EXTENSION_PAGE_ACTION_VISIBILITY_CHANGED,
-        content::Source<ExtensionAction>(page_action_),
-        content::Details<WebContents>(contents));
   }
+  // The pixbuf might not be loaded yet.
+  if (pixbuf)
+    gtk_image_set_from_pixbuf(GTK_IMAGE(image_.get()), pixbuf);

[Evan Stade, 2012/05/16 21:58:17]

please add some more vertical space somewhere as this is a huge block of code
with no empty lines

[not at google - send to devlin, 2012/05/17 01:30:35]

Done.

 }
 
 void LocationBarViewGtk::PageActionViewGtk::OnImageLoaded(