Ensure Button::StateChanged() overrides are sane.

Have each call its superclass explicitly.  Replace one that's better-
suited to being a property observer.  Misc. other cleanup.

Bug: none
Change-Id: I17275c123fa262aa5700b818507f79b5c43b52e7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2292825Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#787391}

ash/app_list/views/search_result_actions_view.cc

@@ -49,9 +49,6 @@ class SearchResultImageButton : public views::ImageButton {
   // ui::EventHandler:
   void OnGestureEvent(ui::GestureEvent* event) override;
 
-  // views::Button:
-  void StateChanged(ButtonState old_state) override;
-
   // views::InkDropHostView:
   std::unique_ptr<views::InkDropRipple> CreateInkDropRipple() const override;
   std::unique_ptr<views::InkDropHighlight> CreateInkDropHighlight()
@@ -125,11 +122,6 @@ void SearchResultImageButton::OnGestureEvent(ui::GestureEvent* event) {
     Button::OnGestureEvent(event);
 }
 
-void SearchResultImageButton::StateChanged(
-    views::Button::ButtonState old_state) {
-  parent_->ActionButtonStateChanged();
-}
-
 std::unique_ptr<views::InkDropRipple>
 SearchResultImageButton::CreateInkDropRipple() const {
   const gfx::Point center = GetLocalBounds().CenterPoint();
@@ -201,7 +193,7 @@ SearchResultActionsView::~SearchResultActionsView() {}
 void SearchResultActionsView::SetActions(const SearchResult::Actions& actions) {
   if (selected_action_.has_value())
     selected_action_.reset();
-  buttons_.clear();
+  subscriptions_.clear();
   RemoveAllChildViews(true);
 
   for (size_t i = 0; i < actions.size(); ++i)
@@ -218,12 +210,8 @@ bool SearchResultActionsView::IsSearchResultHoveredOrSelected() const {
 }
 
 void SearchResultActionsView::UpdateButtonsOnStateChanged() {
-  for (SearchResultImageButton* button : buttons_)
-    button->UpdateOnStateChanged();
-}
-
-void SearchResultActionsView::ActionButtonStateChanged() {
-  UpdateButtonsOnStateChanged();
+  for (views::View* child : children())
+    static_cast<SearchResultImageButton*>(child)->UpdateOnStateChanged();
 }
 
 const char* SearchResultActionsView::GetClassName() const {
@@ -272,9 +260,9 @@ void SearchResultActionsView::NotifyA11yResultSelected() {
   DCHECK(HasSelectedAction());
 
   int selected_action = GetSelectedAction();
-  for (views::Button* button : buttons_) {
-    if (button->tag() == selected_action) {
-      button->NotifyAccessibilityEvent(ax::mojom::Event::kSelection, true);
+  for (views::View* child : children()) {
+    if (static_cast<views::Button*>(child)->tag() == selected_action) {
+      child->NotifyAccessibilityEvent(ax::mojom::Event::kSelection, true);
       return;
     }
   }
@@ -296,14 +284,16 @@ bool SearchResultActionsView::HasSelectedAction() const {
 void SearchResultActionsView::CreateImageButton(
     const SearchResult::Action& action,
     int action_index) {
-  SearchResultImageButton* button = new SearchResultImageButton(this, action);
+  auto* const button =
+      AddChildView(std::make_unique<SearchResultImageButton>(this, action));
   button->set_tag(action_index);
-  AddChildView(button);
-  buttons_.emplace_back(button);
+  subscriptions_.push_back(button->AddStateChangedCallback(
+      base::BindRepeating(&SearchResultActionsView::UpdateButtonsOnStateChanged,
+                          base::Unretained(this))));
 }
 
 size_t SearchResultActionsView::GetActionCount() const {
-  return buttons_.size();
+  return children().size();
 }
 
 void SearchResultActionsView::ChildVisibilityChanged(views::View* child) {

ash/app_list/views/search_result_actions_view.h

@@ -5,7 +5,7 @@
 #ifndef ASH_APP_LIST_VIEWS_SEARCH_RESULT_ACTIONS_VIEW_H_
 #define ASH_APP_LIST_VIEWS_SEARCH_RESULT_ACTIONS_VIEW_H_
 
-#include <vector>
+#include <list>
 
 #include "ash/app_list/app_list_export.h"
 #include "ash/app_list/model/search/search_result.h"
@@ -17,7 +17,6 @@ namespace ash {
 
 class SearchResultActionsViewDelegate;
 class SearchResultView;
-class SearchResultImageButton;
 
 // SearchResultActionsView displays a SearchResult::Actions in a button
 // strip. Each action is presented as a button and horizontally laid out.
@@ -36,9 +35,6 @@ class APP_LIST_EXPORT SearchResultActionsView : public views::View,
   // Updates the button UI upon the SearchResultView's UI state change.
   void UpdateButtonsOnStateChanged();
 
-  // Called when one of the action buttons changes state.
-  void ActionButtonStateChanged();
-
   // views::View:
   const char* GetClassName() const override;
 
@@ -88,7 +84,7 @@ class APP_LIST_EXPORT SearchResultActionsView : public views::View,
   base::Optional<int> selected_action_;
 
   SearchResultActionsViewDelegate* delegate_;  // Not owned.
-  std::vector<SearchResultImageButton*> buttons_;
+  std::list<views::PropertyChangedSubscription> subscriptions_;
 
   DISALLOW_COPY_AND_ASSIGN(SearchResultActionsView);
 };

ash/app_list/views/search_result_tile_item_view.cc

@@ -299,6 +299,7 @@ bool SearchResultTileItemView::OnKeyPressed(const ui::KeyEvent& event) {
 }
 
 void SearchResultTileItemView::StateChanged(ButtonState old_state) {
+  SearchResultBaseView::StateChanged(old_state);
   UpdateBackgroundColor();
 }
 

ash/login/ui/login_password_view.cc

@@ -269,31 +269,25 @@ class LoginPasswordView::EasyUnlockIcon : public views::Button,
 
   // views::Button:
   void StateChanged(ButtonState old_state) override {
+    Button::StateChanged(old_state);
+
     // Stop showing tooltip, as we most likely exited hover state.
     invoke_hover_.Stop();
 
-    switch (state()) {
-      case ButtonState::STATE_NORMAL:
-        UpdateImage(false /*changed_states*/);
-        break;
-      case ButtonState::STATE_HOVERED:
-        UpdateImage(false /*changed_states*/);
-        if (immediately_hover_for_test_) {
-          on_hovered_.Run();
-        } else {
-          invoke_hover_.Start(
-              FROM_HERE,
-              base::TimeDelta::FromMilliseconds(kDelayBeforeShowingTooltipMs),
-              on_hovered_);
-        }
-        break;
-      case ButtonState::STATE_PRESSED:
-        UpdateImage(false /*changed_states*/);
-        break;
-      case ButtonState::STATE_DISABLED:
-        break;
-      case ButtonState::STATE_COUNT:
-        break;
+    if (state() == ButtonState::STATE_DISABLED)
+      return;
+
+    UpdateImage(false /*changed_states*/);
+
+    if (state() == ButtonState::STATE_HOVERED) {
+      if (immediately_hover_for_test_) {
+        on_hovered_.Run();
+      } else {
+        invoke_hover_.Start(
+            FROM_HERE,
+            base::TimeDelta::FromMilliseconds(kDelayBeforeShowingTooltipMs),
+            on_hovered_);
+      }
     }
   }
 

ash/login/ui/note_action_launch_button.cc

@@ -221,6 +221,7 @@ class NoteActionLaunchButton::ActionButton : public views::ImageButton,
     canvas->Restore();
   }
   void StateChanged(ButtonState old_state) override {
+    ImageButton::StateChanged(old_state);
     UpdateBubbleRadiusAndOpacity();
   }
   void OnFocus() override {

ash/quick_answers/ui/quick_answers_view.cc

@@ -215,18 +215,10 @@ std::vector<views::View*> QuickAnswersView::GetFocusableViews() {
 }
 
 void QuickAnswersView::StateChanged(views::Button::ButtonState old_state) {
-  switch (state()) {
-    case Button::ButtonState::STATE_NORMAL: {
-      SetBackgroundState(false);
-      break;
-    }
-    case Button::ButtonState::STATE_HOVERED: {
-      SetBackgroundState(true);
-      break;
-    }
-    default:
-      break;
-  }
+  Button::StateChanged(old_state);
+  const bool hovered = state() == Button::STATE_HOVERED;
+  if (hovered || (state() == Button::STATE_NORMAL))
+    SetBackgroundState(hovered);
 }
 
 void QuickAnswersView::ButtonPressed(views::Button* sender,

chrome/browser/chromeos/input_method/ui/candidate_view.cc

@@ -200,6 +200,7 @@ void CandidateView::SetHighlighted(bool highlighted) {
 }
 
 void CandidateView::StateChanged(ButtonState old_state) {
+  Button::StateChanged(old_state);
   int text_style = state() == STATE_DISABLED ? views::style::STYLE_DISABLED
                                              : views::style::STYLE_PRIMARY;
   shortcut_label_->SetEnabledColor(views::style::GetColor(

chrome/browser/ui/views/omnibox/omnibox_tab_switch_button.cc

@@ -70,13 +70,13 @@ OmniboxTabSwitchButton::OmniboxTabSwitchButton(
 OmniboxTabSwitchButton::~OmniboxTabSwitchButton() = default;
 
 void OmniboxTabSwitchButton::StateChanged(ButtonState old_state) {
+  MdTextButton::StateChanged(old_state);
   if (state() == STATE_NORMAL && old_state == STATE_PRESSED) {
     SetMouseHandler(parent());
     if (popup_contents_view_->model()->selected_line_state() ==
         OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH)
       popup_contents_view_->UnselectButton();
   }
-  MdTextButton::StateChanged(old_state);
 }
 
 void OmniboxTabSwitchButton::OnThemeChanged() {

chrome/browser/ui/views/payments/payment_request_row_view.cc

@@ -71,8 +71,8 @@ void PaymentRequestRowView::SetIsHighlighted(bool highlighted) {
   }
 }
 
-// views::Button:
 void PaymentRequestRowView::StateChanged(ButtonState old_state) {
+  Button::StateChanged(old_state);
   if (!clickable())
     return;
 

ui/views/controls/button/button.h

@@ -257,7 +257,6 @@ class VIEWS_EXPORT Button : public InkDropHostView,
   // the current node_data. Button's implementation of StateChanged() does
   // nothing; this method is provided for subclasses that wish to do something
   // on state changes.
-  // TODO(pkasting): Replace overrides with property change callbacks.
   virtual void StateChanged(ButtonState old_state);
 
   // Returns true if the event is one that can trigger notifying the listener.