Revert half of the changes from 137638 that are probably causing breakages in

the extension updating flow for GTK.

The reverted changes are those which make PageActionController only return
the visible page actions, rather than returning them all and the UI code hiding
any which aren't visible. It occurred to me later that this isn't even a particularly
good change, and no longer necessary for why I changed it in the first place.

R=mpcomplete@chromium.org
TBR=estade@chromium.org
BUG=129096,129193

Review URL: https://chromiumcodereview.appspot.com/10446005

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

8 files changed, 107 insertions, 156 deletions

diff --git a/chrome/browser/extensions/action_box_controller.cc b/chrome/browser/extensions/action_box_controller.cc
deleted file mode 100644
index 13a8f17..0000000
--- a/chrome/browser/extensions/action_box_controller.cc
+++ /dev/null
@@ -1,23 +0,0 @@
-// Copyright (c) 2012 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/extensions/action_box_controller.h"
-
-#include <algorithm>
-
-namespace extensions {
-
-// static
-void ActionBoxController::AddMissingActions(
-    const std::set<ExtensionAction*>& actions,
-    std::vector<ExtensionAction*>* out) {
-  for (std::set<ExtensionAction*>::const_iterator it = actions.begin();
-       it != actions.end(); ++it) {
-    ExtensionAction* action = (*it);
-    if (!std::count(out->begin(), out->end(), action))
-      out->push_back(action);
-  }
-}
-
-}  // namespace extensions
diff --git a/chrome/browser/extensions/extension_tab_helper.cc b/chrome/browser/extensions/extension_tab_helper.cc
index c7c1c5c..d9d0efa 100644
--- a/chrome/browser/extensions/extension_tab_helper.cc
+++ b/chrome/browser/extensions/extension_tab_helper.cc
@@ -53,7 +53,7 @@ ExtensionTabHelper::ExtensionTabHelper(TabContentsWrapper* wrapper)
     script_badge_controller_.reset(new ScriptBadgeController(wrapper));
   } else {
     script_executor_.reset(new ScriptExecutorImpl(wrapper->web_contents()));
-    action_box_controller_.reset(new PageActionController(wrapper, this));
+    action_box_controller_.reset(new PageActionController(wrapper));
   }
 }
 
@@ -66,26 +66,14 @@ void ExtensionTabHelper::CopyStateFrom(const ExtensionTabHelper& source) {
 }
 
 void ExtensionTabHelper::PageActionStateChanged() {
-  // TODO(kalman): replace this with just the Observer interface.
   web_contents()->NotifyNavigationStateChanged(
       content::INVALIDATE_TYPE_PAGE_ACTIONS);
-
-  FOR_EACH_OBSERVER(Observer, observers_, OnPageActionStateChanged());
 }
 
 void ExtensionTabHelper::GetApplicationInfo(int32 page_id) {
   Send(new ExtensionMsg_GetApplicationInfo(routing_id(), page_id));
 }
 
-void ExtensionTabHelper::AddObserver(ExtensionTabHelper::Observer* observer) {
-  observers_.AddObserver(observer);
-}
-
-void ExtensionTabHelper::RemoveObserver(
-    ExtensionTabHelper::Observer* observer) {
-  observers_.RemoveObserver(observer);
-}
-
 void ExtensionTabHelper::SetExtensionApp(const Extension* extension) {
   DCHECK(!extension || extension->GetFullLaunchURL().is_valid());
   extension_app_ = extension;
diff --git a/chrome/browser/extensions/extension_tab_helper.h b/chrome/browser/extensions/extension_tab_helper.h
index d4a4402..7046863 100644
--- a/chrome/browser/extensions/extension_tab_helper.h
+++ b/chrome/browser/extensions/extension_tab_helper.h
@@ -40,15 +40,6 @@ class ExtensionTabHelper
       public AppNotifyChannelSetup::Delegate,
       public base::SupportsWeakPtr<ExtensionTabHelper> {
  public:
-  class Observer {
-   public:
-    // Called when the page action state (such as visibility, title) changes.
-    virtual void OnPageActionStateChanged() = 0;
-
-   protected:
-    virtual ~Observer() {}
-  };
-
   explicit ExtensionTabHelper(TabContentsWrapper* wrapper);
   virtual ~ExtensionTabHelper();
 
@@ -66,10 +57,6 @@ class ExtensionTabHelper
   // the data is available.
   void GetApplicationInfo(int32 page_id);
 
-  // Observer management.
-  void AddObserver(Observer* observer);
-  void RemoveObserver(Observer* observer);
-
   // App extensions ------------------------------------------------------------
 
   // Sets the extension denoting this as an app. If |extension| is non-null this
diff --git a/chrome/browser/extensions/page_action_controller.cc b/chrome/browser/extensions/page_action_controller.cc
index a8381e4..fc131dd 100644
--- a/chrome/browser/extensions/page_action_controller.cc
+++ b/chrome/browser/extensions/page_action_controller.cc
@@ -8,7 +8,6 @@
 #include "chrome/browser/extensions/extension_browser_event_router.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
-#include "chrome/browser/extensions/extension_tab_helper.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/common/extensions/extension_set.h"
 #include "chrome/common/chrome_notification_types.h"
@@ -17,27 +16,20 @@
 
 namespace extensions {
 
-PageActionController::PageActionController(TabContentsWrapper* tab_contents,
-                                           ExtensionTabHelper* tab_helper)
-    : tab_contents_(tab_contents),
-      tab_helper_(tab_helper) {
-  tab_helper->AddObserver(this);
-}
+PageActionController::PageActionController(TabContentsWrapper* tab_contents)
+    : tab_contents_(tab_contents) {}
 
-PageActionController::~PageActionController() {
-  tab_helper_->RemoveObserver(this);
-}
+PageActionController::~PageActionController() {}
 
 scoped_ptr<std::vector<ExtensionAction*> >
 PageActionController::GetCurrentActions() {
-  int tab_id = ExtensionTabUtil::GetTabId(tab_contents_->web_contents());
   const ExtensionSet* extensions = GetExtensionService()->extensions();
   scoped_ptr<std::vector<ExtensionAction*> > current_actions(
       new std::vector<ExtensionAction*>());
   for (ExtensionSet::const_iterator i = extensions->begin();
        i != extensions->end(); ++i) {
     ExtensionAction* action = (*i)->page_action();
-    if (action && action->GetIsVisible(tab_id))
+    if (action)
       current_actions->push_back(action);
   }
   return current_actions.Pass();
@@ -75,25 +67,6 @@ ActionBoxController::Action PageActionController::OnClicked(
   return ACTION_NONE;
 }
 
-void PageActionController::OnPageActionStateChanged() {
-  content::NotificationService::current()->Notify(
-      chrome::NOTIFICATION_EXTENSION_ACTION_BOX_UPDATED,
-      content::Source<Profile>(tab_contents_->profile()),
-      content::Details<TabContentsWrapper>(tab_contents_));
-
-  // TODO(kalman): remove this, and all occurrences of
-  // NOTIFICATION_EXTENSION_PAGE_ACTION_VISIBILITY_CHANGED, when views and
-  // cocoa have been updated to not use it.
-  //
-  // Only tests care about them, and they only ever use AllSources, so it can
-  // safely be a bogus value.
-  ExtensionAction bogus_action("");
-  content::NotificationService::current()->Notify(
-      chrome::NOTIFICATION_EXTENSION_PAGE_ACTION_VISIBILITY_CHANGED,
-      content::Source<ExtensionAction>(&bogus_action),
-      content::Details<content::WebContents>(tab_contents_->web_contents()));
-}
-
 ExtensionService* PageActionController::GetExtensionService() {
   return ExtensionSystem::Get(tab_contents_->profile())->extension_service();
 }
diff --git a/chrome/browser/extensions/page_action_controller.h b/chrome/browser/extensions/page_action_controller.h
index cdb43a3..bfa3d0f 100644
--- a/chrome/browser/extensions/page_action_controller.h
+++ b/chrome/browser/extensions/page_action_controller.h
@@ -11,7 +11,6 @@
 
 #include "base/observer_list.h"
 #include "chrome/browser/extensions/action_box_controller.h"
-#include "chrome/browser/extensions/extension_tab_helper.h"
 
 class ExtensionService;
 class TabContentsWrapper;
@@ -19,11 +18,9 @@ class TabContentsWrapper;
 namespace extensions {
 
 // An ActionBoxController which corresponds to the page actions of an extension.
-class PageActionController : public ActionBoxController,
-                             public ExtensionTabHelper::Observer {
+class PageActionController : public ActionBoxController {
  public:
-  PageActionController(TabContentsWrapper* tab_contents,
-                       ExtensionTabHelper* tab_helper);
+  explicit PageActionController(TabContentsWrapper* tab_contents);
   virtual ~PageActionController();
 
   // ActionBoxController implementation.
@@ -32,17 +29,12 @@ class PageActionController : public ActionBoxController,
   virtual Action OnClicked(const std::string& extension_id,
                            int mouse_button) OVERRIDE;
 
-  // ExtensionTabHelper::Observer implementation.
-  virtual void OnPageActionStateChanged() OVERRIDE;
-
  private:
   // Gets the ExtensionService for |tab_contents_|.
   ExtensionService* GetExtensionService();
 
   TabContentsWrapper* tab_contents_;
 
-  ExtensionTabHelper* tab_helper_;
-
   DISALLOW_COPY_AND_ASSIGN(PageActionController);
 };
 
diff --git a/chrome/browser/ui/gtk/location_bar_view_gtk.cc b/chrome/browser/ui/gtk/location_bar_view_gtk.cc
index c203756..7a13f81 100644
--- a/chrome/browser/ui/gtk/location_bar_view_gtk.cc
+++ b/chrome/browser/ui/gtk/location_bar_view_gtk.cc
@@ -24,6 +24,7 @@
 #include "chrome/browser/command_updater.h"
 #include "chrome/browser/content_settings/tab_specific_content_settings.h"
 #include "chrome/browser/defaults.h"
+#include "chrome/browser/extensions/action_box_controller.h"
 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/api/commands/command_service_factory.h"
 #include "chrome/browser/extensions/extension_browser_event_router.h"
@@ -360,7 +361,6 @@ void LocationBarViewGtk::Init(bool popup_window_mode) {
   registrar_.Add(this,
                  chrome::NOTIFICATION_EXTENSION_ACTION_BOX_UPDATED,
                  content::Source<Profile>(browser()->profile()));
-
   edit_bookmarks_enabled_.Init(prefs::kEditBookmarksEnabled,
                                profile->GetPrefs(), this);
 
@@ -449,10 +449,16 @@ void LocationBarViewGtk::SetPreviewEnabledPageAction(
     ExtensionAction *page_action,
     bool preview_enabled) {
   DCHECK(page_action);
-  if (preview_enabled && preview_enabled_actions_.insert(page_action).second)
-    UpdatePageActions();
-  if (!preview_enabled && preview_enabled_actions_.erase(page_action) > 0)
-    UpdatePageActions();
+  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;
+    }
+  }
 }
 
 GtkWidget* LocationBarViewGtk::GetPageActionWidget(
@@ -693,11 +699,6 @@ void LocationBarViewGtk::UpdatePageActions() {
     page_actions.swap(*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()) {
@@ -720,7 +721,7 @@ void LocationBarViewGtk::UpdatePageActions() {
     GURL url = browser()->GetSelectedWebContents()->GetURL();
 
     for (size_t i = 0; i < page_action_views_.size(); i++) {
-      page_action_views_[i]->Update(
+      page_action_views_[i]->UpdateVisibility(
           toolbar_model_->input_in_progress() ? NULL : contents, url);
     }
   }
@@ -783,7 +784,16 @@ ExtensionAction* LocationBarViewGtk::GetPageAction(size_t index) {
 }
 
 ExtensionAction* LocationBarViewGtk::GetVisiblePageAction(size_t index) {
-  return page_action_views_[index]->page_action();
+  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;
 }
 
 void LocationBarViewGtk::TestPageActionPressed(size_t index) {
@@ -1508,7 +1518,8 @@ LocationBarViewGtk::PageActionViewGtk::PageActionViewGtk(
       tracker_(this),
       current_tab_id_(-1),
       window_(NULL),
-      accel_group_(NULL) {
+      accel_group_(NULL),
+      preview_enabled_(false) {
   event_box_.Own(gtk_event_box_new());
   gtk_widget_set_size_request(event_box_.get(),
                               Extension::kPageActionIconMaxSize,
@@ -1525,7 +1536,6 @@ 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(),
@@ -1564,56 +1574,75 @@ LocationBarViewGtk::PageActionViewGtk::~PageActionViewGtk() {
     g_object_unref(last_icon_pixbuf_);
 }
 
-void LocationBarViewGtk::PageActionViewGtk::Update(
+bool LocationBarViewGtk::PageActionViewGtk::IsVisible() {
+  return gtk_widget_get_visible(widget());
+}
+
+void LocationBarViewGtk::PageActionViewGtk::UpdateVisibility(
     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;
 
-  // 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;
+  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;
+      }
     }
+    // The pixbuf might not be loaded yet.
+    if (pixbuf)
+      gtk_image_set_from_pixbuf(GTK_IMAGE(image_.get()), pixbuf);
   }
 
-  // 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));
+  }
 }
 
 void LocationBarViewGtk::PageActionViewGtk::OnImageLoaded(
diff --git a/chrome/browser/ui/gtk/location_bar_view_gtk.h b/chrome/browser/ui/gtk/location_bar_view_gtk.h
index 3f3d4d8..4a0fbce 100644
--- a/chrome/browser/ui/gtk/location_bar_view_gtk.h
+++ b/chrome/browser/ui/gtk/location_bar_view_gtk.h
@@ -9,7 +9,6 @@
 #include <gtk/gtk.h>
 
 #include <map>
-#include <set>
 #include <string>
 
 #include "base/basictypes.h"
@@ -19,7 +18,6 @@
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/autocomplete/autocomplete_edit.h"
 #include "chrome/browser/command_updater.h"
-#include "chrome/browser/extensions/action_box_controller.h"
 #include "chrome/browser/extensions/extension_context_menu_model.h"
 #include "chrome/browser/extensions/image_loading_tracker.h"
 #include "chrome/browser/prefs/pref_member.h"
@@ -80,9 +78,10 @@ class LocationBarViewGtk : public AutocompleteEditController,
   // Returns the current WebContents.
   content::WebContents* GetWebContents() const;
 
-  // If |preview_enabled| is true, the view will display the
-  // This is used by the ExtensionInstalledBubbleGtk to preview what the icon
+  // Sets |preview_enabled| for the PageActionViewGtk associated with this
+  // |page_action|. If |preview_enabled| is true, the view will display the
   // page action's icon even though it has not been activated by the extension.
+  // This is used by the ExtensionInstalledBubbleGtk to preview what the icon
   // will look like for the user upon installation of the extension.
   void SetPreviewEnabledPageAction(ExtensionAction *page_action,
                                    bool preview_enabled);
@@ -229,10 +228,16 @@ class LocationBarViewGtk : public AutocompleteEditController,
 
     ExtensionAction* page_action() { return page_action_; }
 
-    // Called to notify the PageAction that it should update based on the state
-    // of |page_action_|. |contents| is the WebContents that is active, |url|
+    void set_preview_enabled(bool preview_enabled) {
+      preview_enabled_ = preview_enabled;
+    }
+
+    bool IsVisible();
+
+    // Called to notify the PageAction that it should determine whether to be
+    // visible or hidden. |contents| is the WebContents that is active, |url|
     // is the current page URL.
-    void Update(content::WebContents* contents, const GURL& url);
+    void UpdateVisibility(content::WebContents* contents, const GURL& url);
 
     // A callback from ImageLoadingTracker for when the image has loaded.
     virtual void OnImageLoaded(const gfx::Image& image,
@@ -311,6 +316,10 @@ class LocationBarViewGtk : public AutocompleteEditController,
     // The keybinding accelerator registered to show the page action popup.
     scoped_ptr<ui::AcceleratorGtk> keybinding_;
 
+    // This is used for post-install visual feedback. The page_action icon
+    // is briefly shown even if it hasn't been enabled by its extension.
+    bool preview_enabled_;
+
     // The context menu view and model for this extension action.
     scoped_ptr<MenuGtk> context_menu_;
     scoped_refptr<ExtensionContextMenuModel> context_menu_model_;
@@ -484,9 +493,6 @@ class LocationBarViewGtk : public AutocompleteEditController,
   // Used to change the visibility of the star decoration.
   BooleanPrefMember edit_bookmarks_enabled_;
 
-  // The extension actions which have "preview enabled".
-  std::set<ExtensionAction*> preview_enabled_actions_;
-
   DISALLOW_COPY_AND_ASSIGN(LocationBarViewGtk);
 };
 
diff --git a/chrome/chrome_browser_extensions.gypi b/chrome/chrome_browser_extensions.gypi
index c7ac5ab..7669b80 100644
--- a/chrome/chrome_browser_extensions.gypi
+++ b/chrome/chrome_browser_extensions.gypi
@@ -53,7 +53,6 @@
       'sources': [
         # All .cc, .h, .m, and .mm files under browser/extensions except for
         # tests and mocks.
-        'browser/extensions/action_box_controller.cc',
         'browser/extensions/action_box_controller.h',
         'browser/extensions/api/api_function.cc',
         'browser/extensions/api/api_function.h',