Highlight all BubbleIconView instances.

Highlights the BubbleIconView whenever the corresponding widget is
visible. This was previously a one-off done for the bookmark star view.

This addresses highlighting the translate icon, bookmark icon, zoom icon
and others in a single place. This makes it less error prone and more
future proof.

BubbleIconView still has to be added as an observer of the bubble widget
(but this is already required to remove button-press highlights when the
widget closes).

Bug: chromium:808273
Change-Id: Ibd4e14b4876be3e79ef8a85a0a9feade65aa5628
Reviewed-on: https://chromium-review.googlesource.com/905408Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537928}

chrome/browser/ui/views/frame/browser_view.cc

@@ -1185,9 +1185,10 @@ autofill::SaveCardBubbleView* BrowserView::ShowSaveCreditCardBubble(
 
   autofill::SaveCardBubbleViews* bubble = new autofill::SaveCardBubbleViews(
       anchor_view, gfx::Point(), web_contents, controller);
-  views::BubbleDialogDelegateView::CreateBubble(bubble);
+  views::Widget* bubble_widget =
+      views::BubbleDialogDelegateView::CreateBubble(bubble);
   if (card_view)
-    card_view->OnBubbleCreated(bubble);
+    card_view->OnBubbleWidgetCreated(bubble_widget);
   bubble->Show(user_gesture ? autofill::SaveCardBubbleViews::USER_GESTURE
                             : autofill::SaveCardBubbleViews::AUTOMATIC);
   return bubble;

chrome/browser/ui/views/location_bar/bubble_icon_view.cc

@@ -29,7 +29,8 @@ void BubbleIconView::Init() {
 }
 
 BubbleIconView::BubbleIconView(CommandUpdater* command_updater, int command_id)
-    : image_(new views::ImageView()),
+    : widget_observer_(this),
+      image_(new views::ImageView()),
       command_updater_(command_updater),
       command_id_(command_id),
       active_(false),
@@ -55,10 +56,17 @@ void BubbleIconView::SetTooltipText(const base::string16& tooltip) {
   image_->SetTooltipText(tooltip);
 }
 
-void BubbleIconView::OnBubbleCreated(LocationBarBubbleDelegateView* bubble) {
-  // This observer is removed when the bubble's widget is destroyed, by
-  // |OnWidgetDestroying|.
-  bubble->GetWidget()->AddObserver(this);
+void BubbleIconView::SetHighlighted(bool bubble_visible) {
+  AnimateInkDrop(bubble_visible ? views::InkDropState::ACTIVATED
+                                : views::InkDropState::DEACTIVATED,
+                 nullptr);
+}
+
+void BubbleIconView::OnBubbleWidgetCreated(views::Widget* bubble_widget) {
+  widget_observer_.SetWidget(bubble_widget);
+
+  if (bubble_widget->IsVisible())
+    SetHighlighted(true);
 }
 
 void BubbleIconView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
@@ -201,17 +209,6 @@ void BubbleIconView::OnGestureEvent(ui::GestureEvent* event) {
   }
 }
 
-void BubbleIconView::OnWidgetDestroying(views::Widget* widget) {
-  widget->RemoveObserver(this);
-}
-
-void BubbleIconView::OnWidgetVisibilityChanged(views::Widget* widget,
-                                               bool visible) {
-  // |widget| is a bubble that has just got shown / hidden.
-  if (!visible)
-    AnimateInkDrop(views::InkDropState::DEACTIVATED, nullptr /* event */);
-}
-
 void BubbleIconView::ExecuteCommand(ExecuteSource source) {
   OnExecuting(source);
   if (command_updater_)
@@ -242,3 +239,24 @@ void BubbleIconView::SetActiveInternal(bool active) {
   active_ = active;
   UpdateIcon();
 }
+
+BubbleIconView::WidgetObserver::WidgetObserver(BubbleIconView* parent)
+    : parent_(parent), scoped_observer_(this) {}
+
+BubbleIconView::WidgetObserver::~WidgetObserver() = default;
+
+void BubbleIconView::WidgetObserver::SetWidget(views::Widget* widget) {
+  scoped_observer_.RemoveAll();
+  scoped_observer_.Add(widget);
+}
+
+void BubbleIconView::WidgetObserver::OnWidgetDestroying(views::Widget* widget) {
+  scoped_observer_.Remove(widget);
+}
+
+void BubbleIconView::WidgetObserver::OnWidgetVisibilityChanged(
+    views::Widget* widget,
+    bool visible) {
+  // |widget| is a bubble that has just got shown / hidden.
+  parent_->SetHighlighted(visible);
+}

chrome/browser/ui/views/location_bar/bubble_icon_view.h

@@ -8,13 +8,13 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "base/scoped_observer.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/views/animation/ink_drop_host_view.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/widget/widget_observer.h"
 
 class CommandUpdater;
-class LocationBarBubbleDelegateView;
 
 namespace gfx {
 struct VectorIcon;
@@ -26,13 +26,13 @@ class BubbleDialogDelegateView;
 
 // Represents an icon on the omnibox that shows a bubble when clicked.
 // TODO(spqchan): Convert this to subclass Button.
-class BubbleIconView : public views::InkDropHostView,
-                       public views::WidgetObserver {
+class BubbleIconView : public views::InkDropHostView {
  public:
   void Init();
 
-  // Invoked when a bubble for this icon is created.
-  void OnBubbleCreated(LocationBarBubbleDelegateView* bubble);
+  // Invoked when a bubble for this icon is created. The BubbleIconView changes
+  // highlights based on this widget's visibility.
+  void OnBubbleWidgetCreated(views::Widget* bubble_widget);
 
   // Returns the bubble instance for the icon.
   virtual views::BubbleDialogDelegateView* GetBubble() const = 0;
@@ -91,10 +91,6 @@ class BubbleIconView : public views::InkDropHostView,
   // ui::EventHandler:
   void OnGestureEvent(ui::GestureEvent* event) override;
 
-  // views::WidgetObserver:
-  void OnWidgetDestroying(views::Widget* widget) override;
-  void OnWidgetVisibilityChanged(views::Widget* widget, bool visible) override;
-
  protected:
   // Calls OnExecuting and runs |command_id_| with a valid |command_updater_|.
   virtual void ExecuteCommand(ExecuteSource source);
@@ -115,6 +111,30 @@ class BubbleIconView : public views::InkDropHostView,
   bool active() const { return active_; }
 
  private:
+  class WidgetObserver : public views::WidgetObserver {
+   public:
+    explicit WidgetObserver(BubbleIconView* parent);
+    ~WidgetObserver() override;
+
+    void SetWidget(views::Widget* widget);
+
+   private:
+    // views::WidgetObserver:
+    void OnWidgetDestroying(views::Widget* widget) override;
+    void OnWidgetVisibilityChanged(views::Widget* widget,
+                                   bool visible) override;
+
+    BubbleIconView* const parent_;
+    ScopedObserver<views::Widget, views::WidgetObserver> scoped_observer_;
+    DISALLOW_COPY_AND_ASSIGN(WidgetObserver);
+  };
+
+  // Highlights the ink drop for the icon, used when the corresponding widget
+  // is visible.
+  void SetHighlighted(bool bubble_visible);
+
+  WidgetObserver widget_observer_;
+
   // The image shown in the button.
   views::ImageView* image_;
 

chrome/browser/ui/views/location_bar/star_view.cc

@@ -30,12 +30,6 @@ StarView::StarView(CommandUpdater* command_updater, Browser* browser)
 
 StarView::~StarView() {}
 
-void StarView::SetHighlighted() {
-  views::InkDrop* ink_drop = GetInkDrop();
-  if (ink_drop && !ink_drop->IsHighlightFadingInOrVisible())
-    AnimateInkDrop(views::InkDropState::ACTIVATED, nullptr);
-}
-
 void StarView::SetToggled(bool on) {
   BubbleIconView::SetActiveInternal(on);
   SetTooltipText(l10n_util::GetStringUTF16(
@@ -45,10 +39,11 @@ void StarView::SetToggled(bool on) {
 void StarView::ShowPromo() {
   BookmarkPromoBubbleView* bookmark_promo_bubble =
       BookmarkPromoBubbleView::CreateOwned(this);
+
+  OnBubbleWidgetCreated(bookmark_promo_bubble->GetWidget());
   if (!bookmark_promo_observer_.IsObserving(
           bookmark_promo_bubble->GetWidget())) {
     bookmark_promo_observer_.Add(bookmark_promo_bubble->GetWidget());
-    AnimateInkDrop(views::InkDropState::ACTIVATED, nullptr);
     SetActiveInternal(false);
     UpdateIcon();
   }
@@ -100,7 +95,6 @@ SkColor StarView::GetInkDropBaseColor() const {
 void StarView::OnWidgetDestroying(views::Widget* widget) {
   if (bookmark_promo_observer_.IsObserving(widget)) {
     bookmark_promo_observer_.Remove(widget);
-    AnimateInkDrop(views::InkDropState::DEACTIVATED, nullptr);
     SetActiveInternal(false);
     UpdateIcon();
   }

chrome/browser/ui/views/location_bar/star_view.h

@@ -11,17 +11,13 @@
 
 class Browser;
 class CommandUpdater;
-class WidgetObserver;
 
 // The star icon to show a bookmark bubble.
-class StarView : public BubbleIconView {
+class StarView : public BubbleIconView, public views::WidgetObserver {
  public:
   StarView(CommandUpdater* command_updater, Browser* browser);
   ~StarView() override;
 
-  // Show the Animated Ink drop highlight.
-  void SetHighlighted();
-
   // Toggles the star on or off.
   void SetToggled(bool on);
 
@@ -31,18 +27,20 @@ class StarView : public BubbleIconView {
  protected:
   // BubbleIconView:
   void OnExecuting(BubbleIconView::ExecuteSource execute_source) override;
-  void OnWidgetDestroying(views::Widget* widget) override;
   void ExecuteCommand(ExecuteSource source) override;
   views::BubbleDialogDelegateView* GetBubble() const override;
   SkColor GetInkDropBaseColor() const override;
   const gfx::VectorIcon& GetVectorIcon() const override;
 
+  // views::WidgetObserver:
+  void OnWidgetDestroying(views::Widget* widget) override;
+
  private:
   Browser* const browser_;
 
   // Observes the BookmarkPromoBubbleView's widget. Used to tell whether the
   // promo is open and gets called back when it closes.
-  ScopedObserver<views::Widget, WidgetObserver> bookmark_promo_observer_;
+  ScopedObserver<views::Widget, views::WidgetObserver> bookmark_promo_observer_;
 
   DISALLOW_COPY_AND_ASSIGN(StarView);
 };

chrome/browser/ui/views/location_bar/zoom_bubble_view.cc

@@ -180,9 +180,9 @@ void ZoomBubbleView::ShowBubble(content::WebContents* web_contents,
 
   views::Widget* zoom_bubble_widget =
       views::BubbleDialogDelegateView::CreateBubble(zoom_bubble_);
-  if (anchor_view) {
-    zoom_bubble_widget->AddObserver(
-        browser_view->GetLocationBarView()->zoom_view());
+  if (zoom_bubble_widget && anchor_view) {
+    browser_view->GetLocationBarView()->zoom_view()->OnBubbleWidgetCreated(
+        zoom_bubble_widget);
   }
 #else
   gfx::NativeView parent =

chrome/browser/ui/views/passwords/manage_passwords_icon_views.cc

@@ -28,11 +28,6 @@ ManagePasswordsIconViews::ManagePasswordsIconViews(CommandUpdater* updater)
 
 ManagePasswordsIconViews::~ManagePasswordsIconViews() {}
 
-void ManagePasswordsIconViews::SetHighlighted() {
-  if (!GetInkDrop()->IsHighlightFadingInOrVisible())
-    AnimateInkDrop(views::InkDropState::ACTIVATED, nullptr);
-}
-
 void ManagePasswordsIconViews::SetState(password_manager::ui::State state) {
   if (state_ == state)
     return;

chrome/browser/ui/views/passwords/manage_passwords_icon_views.h

@@ -21,8 +21,6 @@ class ManagePasswordsIconViews : public ManagePasswordsIconView,
   explicit ManagePasswordsIconViews(CommandUpdater* updater);
   ~ManagePasswordsIconViews() override;
 
-  void SetHighlighted();
-
   // ManagePasswordsIconView:
   void SetState(password_manager::ui::State state) override;
 

chrome/browser/ui/views/passwords/password_bubble_view_base.cc

@@ -60,10 +60,9 @@ void PasswordBubbleViewBase::ShowBubble(content::WebContents* web_contents,
       views::BubbleDialogDelegateView::CreateBubble(g_manage_passwords_bubble_);
 
   if (anchor_view) {
-    ManagePasswordsIconViews* const icon =
-        browser_view->GetLocationBarView()->manage_passwords_icon_view();
-    bubble_widget->AddObserver(icon);
-    icon->SetHighlighted();
+    browser_view->GetLocationBarView()
+        ->manage_passwords_icon_view()
+        ->OnBubbleWidgetCreated(bubble_widget);
   }
 
   // Adjust for fullscreen after creation as it relies on the content size.

chrome/browser/ui/views/toolbar/toolbar_view.cc

@@ -274,7 +274,7 @@ void ToolbarView::ShowIntentPickerBubble(
         intent_picker_view, GetWebContents(), app_info,
         false /* disable_stay_in_chrome */, callback);
     if (bubble_widget && intent_picker_view)
-      bubble_widget->AddObserver(intent_picker_view);
+      intent_picker_view->OnBubbleWidgetCreated(bubble_widget);
   }
 }
 #endif  // defined(OS_CHROMEOS)
@@ -284,7 +284,7 @@ void ToolbarView::ShowBookmarkBubble(
     bool already_bookmarked,
     bookmarks::BookmarkBubbleObserver* observer) {
   views::View* anchor_view = location_bar();
-  StarView* star_view = location_bar()->star_view();
+  BubbleIconView* const star_view = location_bar()->star_view();
   if (!ui::MaterialDesignController::IsSecondaryUiMaterial()) {
     if (star_view && star_view->visible())
       anchor_view = star_view;
@@ -297,10 +297,8 @@ void ToolbarView::ShowBookmarkBubble(
   views::Widget* bubble_widget = BookmarkBubbleView::ShowBubble(
       anchor_view, gfx::Rect(), nullptr, observer, std::move(delegate),
       browser_->profile(), url, already_bookmarked);
-  if (bubble_widget && star_view) {
-    star_view->SetHighlighted();
-    bubble_widget->AddObserver(star_view);
-  }
+  if (bubble_widget && star_view)
+    star_view->OnBubbleWidgetCreated(bubble_widget);
 }
 
 void ToolbarView::ShowTranslateBubble(
@@ -322,7 +320,7 @@ void ToolbarView::ShowTranslateBubble(
       is_user_gesture ? TranslateBubbleView::USER_GESTURE
                       : TranslateBubbleView::AUTOMATIC);
   if (bubble_widget && translate_icon_view)
-    bubble_widget->AddObserver(translate_icon_view);
+    translate_icon_view->OnBubbleWidgetCreated(bubble_widget);
 }
 
 int ToolbarView::GetMaxBrowserActionsWidth() const {