Abstract away how the toolbar icon container observes changes.

This will allow changing the observation method in the future without
affecting the callers.  This does require changing the pointer type in
PageActionIconParams to a concrete type, but that's OK for now; if we
really need to later, we could introduce a new abstract class that
simply exposes an ObserveButton() method.

Bug: none
Change-Id: I540905bf89d0d05ecb69806495d9a3618b21a19f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2274842
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#784270}

chrome/browser/ui/views/extensions/extensions_toolbar_container.cc

@@ -471,8 +471,7 @@ void ExtensionsToolbarContainer::CreateActionForId(
   auto icon = std::make_unique<ToolbarActionView>(actions_.back().get(), this);
   // Set visibility before adding to prevent extraneous animation.
   icon->SetVisible(CanShowIconInToolbar() && model_->IsActionPinned(action_id));
-  icon->AddButtonObserver(this);
-  icon->AddObserver(this);
+  ObserveButton(icon.get());
   icons_.insert({action_id, AddChildView(std::move(icon))});
 }
 

chrome/browser/ui/views/page_action/page_action_icon_controller.cc

@@ -26,6 +26,7 @@
 #include "chrome/browser/ui/views/send_tab_to_self/send_tab_to_self_icon_view.h"
 #include "chrome/browser/ui/views/sharing/sharing_dialog_view.h"
 #include "chrome/browser/ui/views/sharing/sharing_icon_view.h"
+#include "chrome/browser/ui/views/toolbar/toolbar_icon_container_view.h"
 #include "chrome/browser/ui/views/translate/translate_icon_view.h"
 #include "ui/views/layout/box_layout.h"
 
@@ -167,9 +168,7 @@ void PageActionIconController::Init(const PageActionIconParams& params,
     if (params.font_list)
       icon->SetFontList(*params.font_list);
     if (params.button_observer)
-      icon->AddButtonObserver(params.button_observer);
-    if (params.view_observer)
-      icon->views::View::AddObserver(params.view_observer);
+      params.button_observer->ObserveButton(icon);
     icon_container_->AddPageActionIcon(icon);
   }
 

chrome/browser/ui/views/page_action/page_action_icon_params.h

@@ -15,16 +15,12 @@
 
 class Browser;
 class CommandUpdater;
+class ToolbarIconContainerView;
 
 namespace gfx {
 class FontList;
 }
 
-namespace views {
-class ButtonObserver;
-class ViewObserver;
-}  // namespace views
-
 struct PageActionIconParams {
   PageActionIconParams();
   ~PageActionIconParams();
@@ -41,8 +37,10 @@ struct PageActionIconParams {
   CommandUpdater* command_updater = nullptr;
   IconLabelBubbleView::Delegate* icon_label_bubble_delegate = nullptr;
   PageActionIconView::Delegate* page_action_icon_delegate = nullptr;
-  views::ButtonObserver* button_observer = nullptr;
-  views::ViewObserver* view_observer = nullptr;
+  // If in the future another class also wants to observe button changes, this
+  // type could be an abstract class that simply exposes an ObserveButton()
+  // method.
+  ToolbarIconContainerView* button_observer = nullptr;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(PageActionIconParams);

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

@@ -46,7 +46,6 @@ ToolbarAccountIconContainerView::ToolbarAccountIconContainerView(
   params.icon_label_bubble_delegate = this;
   params.page_action_icon_delegate = this;
   params.button_observer = this;
-  params.view_observer = this;
   AddMainButton(avatar_);
 
   // Since the insertion point for icons before the avatar button, we don't

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

@@ -46,12 +46,16 @@ ToolbarIconContainerView::~ToolbarIconContainerView() {
 
 void ToolbarIconContainerView::AddMainButton(views::Button* main_button) {
   DCHECK(!main_button_);
-  main_button->AddObserver(this);
-  main_button->AddButtonObserver(this);
   main_button_ = main_button;
+  ObserveButton(main_button_);
   AddChildView(main_button_);
 }
 
+void ToolbarIconContainerView::ObserveButton(views::Button* button) {
+  button->AddButtonObserver(this);
+  button->AddObserver(this);
+}
+
 void ToolbarIconContainerView::AddObserver(Observer* obs) {
   observers_.AddObserver(obs);
 }
@@ -60,6 +64,22 @@ void ToolbarIconContainerView::RemoveObserver(const Observer* obs) {
   observers_.RemoveObserver(obs);
 }
 
+void ToolbarIconContainerView::OverrideIconColor(SkColor color) {
+  icon_color_ = color;
+  UpdateAllIcons();
+}
+
+SkColor ToolbarIconContainerView::GetIconColor() const {
+  if (icon_color_)
+    return icon_color_.value();
+  return GetThemeProvider()->GetColor(
+      ThemeProperties::COLOR_TOOLBAR_BUTTON_ICON);
+}
+
+bool ToolbarIconContainerView::IsHighlighted() {
+  return ShouldDisplayHighlight();
+}
+
 void ToolbarIconContainerView::OnHighlightChanged(
     views::Button* observed_button,
     bool highlighted) {
@@ -67,12 +87,10 @@ void ToolbarIconContainerView::OnHighlightChanged(
   if (observed_button == main_button_)
     return;
 
-  if (highlighted) {
-    DCHECK(observed_button);
+  if (highlighted)
     highlighted_buttons_.insert(observed_button);
-  } else {
+  else
     highlighted_buttons_.erase(observed_button);
-  }
 
   UpdateHighlight();
 }
@@ -110,6 +128,15 @@ const char* ToolbarIconContainerView::GetClassName() const {
   return kToolbarIconContainerViewClassName;
 }
 
+void ToolbarIconContainerView::AnimationProgressed(
+    const gfx::Animation* animation) {
+  SetHighlightBorder();
+}
+
+void ToolbarIconContainerView::AnimationEnded(const gfx::Animation* animation) {
+  SetHighlightBorder();
+}
+
 bool ToolbarIconContainerView::ShouldDisplayHighlight() {
   if (!uses_highlight_)
     return false;
@@ -153,22 +180,6 @@ void ToolbarIconContainerView::UpdateHighlight() {
     observer.OnHighlightChanged();
 }
 
-void ToolbarIconContainerView::OverrideIconColor(SkColor color) {
-  icon_color_ = color;
-  UpdateAllIcons();
-}
-
-SkColor ToolbarIconContainerView::GetIconColor() const {
-  if (icon_color_)
-    return icon_color_.value();
-  return GetThemeProvider()->GetColor(
-      ThemeProperties::COLOR_TOOLBAR_BUTTON_ICON);
-}
-
-bool ToolbarIconContainerView::IsHighlighted() {
-  return ShouldDisplayHighlight();
-}
-
 void ToolbarIconContainerView::SetHighlightBorder() {
   const float highlight_value = highlight_animation_.GetCurrentValue();
   if (highlight_value > 0.0f) {
@@ -183,12 +194,3 @@ void ToolbarIconContainerView::SetHighlightBorder() {
     SetBorder(nullptr);
   }
 }
-
-void ToolbarIconContainerView::AnimationProgressed(
-    const gfx::Animation* animation) {
-  SetHighlightBorder();
-}
-
-void ToolbarIconContainerView::AnimationEnded(const gfx::Animation* animation) {
-  SetHighlightBorder();
-}

chrome/browser/ui/views/toolbar/toolbar_icon_container_view.h

@@ -35,6 +35,10 @@ class ToolbarIconContainerView : public views::View,
   // Adds the RHS child as well as setting its margins.
   void AddMainButton(views::Button* main_button);
 
+  // Begins observing |button| for changes that should affect the container's
+  // highlight state.
+  void ObserveButton(views::Button* button);
+
   void AddObserver(Observer* obs);
   void RemoveObserver(const Observer* obs);