Merge "Fix disappearing page actions bug." to 2214.

> Fix disappearing page actions bug.
>
> The logic in LocationBarViewMac implicitly relied on the assumption that there
> is a 1:1 correspondence between |page_actions_| and |page_action_decorations_|.
> The implementation of DeletePageActionDecorations() cleared one vector but not
> the other, breaking this implicit assumption and causing the bug. This CL
> removes |page_actions_|, as the information is already contained within
> |page_action_decorations_|, which fixes the bug.
>
> The same was true for LocationBarView, modulo some minor renaming of members
> and methods. There was a second bug in LocationBarView, where
> InvalidatePageActions() deleted the page actions instead of refreshing them.
>
> The documentation for InvalidatePageActions() and UpdatePageActions() implied
> that the two methods were called at different times, and should have different
> effects. Looking at the call sites, UpdatePageActions() was being called in
> places where the documentation implied InvalidatePageActions() should be
> called. This CL deletes InvalidatePageActions().
>
> BUG=173055
> TBR=jcivelli@chromium.org
>
> Review URL: https://codereview.chromium.org/789763004
>
> Cr-Commit-Position: refs/heads/master@{#309797}

(cherry picked from commit 545918a1b072eed5e15f28c772c682f9167b6677)
BUG=173055
TBR=jcivelli@chromium.org, danduong@chromium.org, rsesek@chromium.org, pkasting@chromium.org, wittman@chromium.org

Review URL: https://codereview.chromium.org/833203002

Cr-Commit-Position: refs/branch-heads/2214@{#380}
Cr-Branched-From: 03655fd3f6d72165dc3c9bd2c89807305316fe6c-refs/heads/master@{#303346}

7 files changed, 54 insertions, 42 deletions

diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index eda6e62..4eb9f58 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -1956,7 +1956,7 @@ void Browser::Observe(int type,
     case extensions::NOTIFICATION_EXTENSION_PROCESS_TERMINATED: {
       Profile* profile = content::Source<Profile>(source).ptr();
       if (profile_->IsSameProfile(profile) && window()->GetLocationBar())
-        window()->GetLocationBar()->InvalidatePageActions();
+        window()->GetLocationBar()->UpdatePageActions();
       break;
     }
 
diff --git a/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h b/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h
index 3d5898b..1b5ed24 100644
--- a/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h
+++ b/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h
@@ -66,7 +66,6 @@ class LocationBarViewMac : public LocationBar,
   void UpdateContentSettingsIcons() override;
   void UpdateManagePasswordsIconAndBubble() override;
   void UpdatePageActions() override;
-  void InvalidatePageActions() override;
   void UpdateBookmarkStarVisibility() override;
   bool ShowPageActionPopup(const extensions::Extension* extension,
                            bool grant_active_tab) override;
@@ -200,6 +199,11 @@ class LocationBarViewMac : public LocationBar,
   // extension service.
   void RefreshPageActionDecorations();
 
+  // Whether the page actions represented by |page_action_decorations_| differ
+  // in ordering or value from |page_actions|.
+  bool PageActionsDiffer(
+      const std::vector<ExtensionAction*>& page_actions) const;
+
   // Updates visibility of the content settings icons based on the current
   // tab contents state.
   bool RefreshContentSettingsDecorations();
@@ -242,9 +246,6 @@ class LocationBarViewMac : public LocationBar,
   // levels.
   scoped_ptr<ZoomDecoration> zoom_decoration_;
 
-  // The installed page actions.
-  std::vector<ExtensionAction*> page_actions_;
-
   // Decorations for the installed Page Actions.
   ScopedVector<PageActionDecoration> page_action_decorations_;
 
diff --git a/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm b/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
index e4c00a8..e71bea5 100644
--- a/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
+++ b/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
@@ -207,11 +207,6 @@ void LocationBarViewMac::UpdatePageActions() {
   [field_ setNeedsDisplay:YES];
 }
 
-void LocationBarViewMac::InvalidatePageActions() {
-  DeletePageActionDecorations();
-  Layout();
-}
-
 void LocationBarViewMac::UpdateBookmarkStarVisibility() {
   star_decoration_->SetVisible(IsStarEnabled());
 }
@@ -668,7 +663,7 @@ void LocationBarViewMac::RefreshPageActionDecorations() {
 
   WebContents* web_contents = GetWebContents();
   if (!web_contents) {
-    DeletePageActionDecorations();  // Necessary?
+    DeletePageActionDecorations();
     return;
   }
 
@@ -676,12 +671,11 @@ void LocationBarViewMac::RefreshPageActionDecorations() {
       extensions::TabHelper::FromWebContents(web_contents)->
           location_bar_controller()->GetCurrentActions();
 
-  if (new_page_actions != page_actions_) {
-    page_actions_.swap(new_page_actions);
+  if (PageActionsDiffer(new_page_actions)) {
     DeletePageActionDecorations();
-    for (size_t i = 0; i < page_actions_.size(); ++i) {
+    for (size_t i = 0; i < new_page_actions.size(); ++i) {
       page_action_decorations_.push_back(
-          new PageActionDecoration(this, browser_, page_actions_[i]));
+          new PageActionDecoration(this, browser_, new_page_actions[i]));
     }
 
     // Move rightmost extensions to the start.
@@ -697,6 +691,20 @@ void LocationBarViewMac::RefreshPageActionDecorations() {
   }
 }
 
+bool LocationBarViewMac::PageActionsDiffer(
+    const std::vector<ExtensionAction*>& page_actions) const {
+  if (page_action_decorations_.size() != page_actions.size())
+    return true;
+
+  for (size_t index = 0; index < page_actions.size(); ++index) {
+    PageActionDecoration* decoration = page_action_decorations_[index];
+    if (decoration->GetPageAction() != page_actions[index])
+      return true;
+  }
+
+  return false;
+}
+
 bool LocationBarViewMac::RefreshContentSettingsDecorations() {
   const bool input_in_progress = GetToolbarModel()->input_in_progress();
   WebContents* web_contents = input_in_progress ?
diff --git a/chrome/browser/ui/location_bar/location_bar.h b/chrome/browser/ui/location_bar/location_bar.h
index 2a05187..df12532 100644
--- a/chrome/browser/ui/location_bar/location_bar.h
+++ b/chrome/browser/ui/location_bar/location_bar.h
@@ -61,10 +61,6 @@ class LocationBar {
   // Updates the visibility of the bookmark star.
   virtual void UpdateBookmarkStarVisibility() = 0;
 
-  // Called when the page-action data needs to be refreshed, e.g. when an
-  // extension is unloaded or crashes.
-  virtual void InvalidatePageActions() = 0;
-
   // Shows the popup for the given |extension| and, if |grant_active_tab| is
   // true, grants the extension active tab permissions.
   // Returns true if a popup was shown.
diff --git a/chrome/browser/ui/views/location_bar/location_bar_view.cc b/chrome/browser/ui/views/location_bar/location_bar_view.cc
index 4cd9598..f94e672 100644
--- a/chrome/browser/ui/views/location_bar/location_bar_view.cc
+++ b/chrome/browser/ui/views/location_bar/location_bar_view.cc
@@ -1082,15 +1082,14 @@ bool LocationBarView::RefreshPageActionViews() {
 
   // 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_ != new_page_actions) {
+  if (PageActionsDiffer(new_page_actions)) {
     changed = true;
 
-    page_actions_.swap(new_page_actions);
-    DeletePageActionViews();  // Delete the old views (if any).
+    DeletePageActionViews();
 
     // Create the page action views.
-    for (PageActions::const_iterator i = page_actions_.begin();
-         i != page_actions_.end(); ++i) {
+    for (PageActions::const_iterator i = new_page_actions.begin();
+         i != new_page_actions.end(); ++i) {
       PageActionWithBadgeView* page_action_view = new PageActionWithBadgeView(
           delegate_->CreatePageActionImageView(this, *i));
       page_action_view->SetVisible(false);
@@ -1117,18 +1116,30 @@ bool LocationBarView::RefreshPageActionViews() {
       AddChildViewAt(*i, GetIndexOf(right_anchor));
   }
 
-  if (!page_action_views_.empty() && web_contents) {
-    for (PageActionViews::const_iterator i(page_action_views_.begin());
-         i != page_action_views_.end(); ++i) {
-      bool old_visibility = (*i)->visible();
-      (*i)->UpdateVisibility(
-          GetToolbarModel()->input_in_progress() ? NULL : web_contents);
-      changed |= old_visibility != (*i)->visible();
-    }
+  for (PageActionViews::const_iterator i(page_action_views_.begin());
+       i != page_action_views_.end(); ++i) {
+    bool old_visibility = (*i)->visible();
+    (*i)->UpdateVisibility(
+        GetToolbarModel()->input_in_progress() ? NULL : web_contents);
+    changed |= old_visibility != (*i)->visible();
   }
   return changed;
 }
 
+bool LocationBarView::PageActionsDiffer(
+    const PageActions& page_actions) const {
+  if (page_action_views_.size() != page_actions.size())
+    return true;
+
+  for (size_t index = 0; index < page_actions.size(); ++index) {
+    PageActionWithBadgeView* view = page_action_views_[index];
+    if (view->image_view()->extension_action() != page_actions[index])
+      return true;
+  }
+
+  return false;
+}
+
 bool LocationBarView::RefreshZoomView() {
   DCHECK(zoom_view_);
   WebContents* web_contents = GetWebContents();
@@ -1293,10 +1304,6 @@ void LocationBarView::UpdatePageActions() {
   }
 }
 
-void LocationBarView::InvalidatePageActions() {
-  DeletePageActionViews();
-}
-
 void LocationBarView::UpdateBookmarkStarVisibility() {
   if (star_view_) {
     star_view_->SetVisible(
diff --git a/chrome/browser/ui/views/location_bar/location_bar_view.h b/chrome/browser/ui/views/location_bar/location_bar_view.h
index c418feb..133a611 100644
--- a/chrome/browser/ui/views/location_bar/location_bar_view.h
+++ b/chrome/browser/ui/views/location_bar/location_bar_view.h
@@ -298,7 +298,8 @@ class LocationBarView : public LocationBar,
   // of at least one of the views in |content_setting_views_| changed.
   bool RefreshContentSettingViews();
 
-  // Deletes all page action views that we have created.
+  // Clears |page_action_views_| and removes the elements from the view
+  // hierarchy.
   void DeletePageActionViews();
 
   // Updates the views for the Page Actions, to reflect state changes for
@@ -306,6 +307,10 @@ class LocationBarView : public LocationBar,
   // changed, or PageActionWithBadgeView were created/destroyed.
   bool RefreshPageActionViews();
 
+  // Whether the page actions represented by |page_action_views_| differ
+  // in ordering or value from |page_actions|.
+  bool PageActionsDiffer(const PageActions& page_actions) const;
+
   // Updates the view for the zoom icon based on the current tab's zoom. Returns
   // true if the visibility of the view changed.
   bool RefreshZoomView();
@@ -348,7 +353,6 @@ class LocationBarView : public LocationBar,
   void UpdateContentSettingsIcons() override;
   void UpdateManagePasswordsIconAndBubble() override;
   void UpdatePageActions() override;
-  void InvalidatePageActions() override;
   void UpdateBookmarkStarVisibility() override;
   bool ShowPageActionPopup(const extensions::Extension* extension,
                            bool grant_active_tab) override;
@@ -470,9 +474,6 @@ class LocationBarView : public LocationBar,
   // The manage passwords icon.
   ManagePasswordsIconView* manage_passwords_icon_view_;
 
-  // The current page actions.
-  PageActions page_actions_;
-
   // The page action icon views.
   PageActionViews page_action_views_;
 
diff --git a/chrome/test/base/test_browser_window.h b/chrome/test/base/test_browser_window.h
index 044a3f9..e97c103 100644
--- a/chrome/test/base/test_browser_window.h
+++ b/chrome/test/base/test_browser_window.h
@@ -171,7 +171,6 @@ class TestBrowserWindow : public BrowserWindow {
     void UpdateContentSettingsIcons() override {}
     void UpdateManagePasswordsIconAndBubble() override {}
     void UpdatePageActions() override {}
-    void InvalidatePageActions() override {}
     void UpdateBookmarkStarVisibility() override {}
     bool ShowPageActionPopup(const extensions::Extension* extension,
                              bool grant_active_tab) override;