Add property support for Button's state.

Also replaces ButtonController::OnStateChanged() with a property change
callback.

This will allow removing ButtonObserver entirely in a future CL, and
potentially all Button::StateChanged() overrides as well.

Bug: none
Change-Id: I143a6e7d0668d8bee7535f3d57bbf04430685428
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2284218
Commit-Queue: Wei Li <weili@chromium.org>
Reviewed-by: Wei Li <weili@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#785940}

ui/views/controls/button/button.cc

@@ -175,6 +175,10 @@ const base::string16& Button::GetAccessibleName() const {
   return accessible_name_.empty() ? tooltip_text_ : accessible_name_;
 }
 
+Button::ButtonState Button::GetState() const {
+  return state_;
+}
+
 void Button::SetState(ButtonState state) {
   if (state == state_)
     return;
@@ -201,7 +205,7 @@ void Button::SetState(ButtonState state) {
   ButtonState old_state = state_;
   state_ = state;
   StateChanged(old_state);
-  SchedulePaint();
+  OnPropertyChanged(&state_, kPropertyEffectsPaint);
 }
 
 Button::ButtonState Button::GetVisualState() const {
@@ -277,6 +281,11 @@ void Button::RemoveButtonObserver(ButtonObserver* observer) {
   button_observers_.RemoveObserver(observer);
 }
 
+PropertyChangedSubscription Button::AddStateChangedCallback(
+    PropertyChangedCallback callback) {
+  return AddPropertyChangedCallback(&state_, std::move(callback));
+}
+
 Button::KeyClickAction Button::GetKeyClickActionForEvent(
     const ui::KeyEvent& event) {
   if (event.key_code() == ui::VKEY_SPACE)
@@ -561,7 +570,6 @@ void Button::OnClickCanceled(const ui::Event& event) {
 void Button::OnSetTooltipText(const base::string16& tooltip_text) {}
 
 void Button::StateChanged(ButtonState old_state) {
-  button_controller_->OnStateChanged(old_state);
   for (ButtonObserver& observer : button_observers_)
     observer.OnStateChanged(this, old_state);
 }
@@ -622,8 +630,16 @@ void Button::WidgetPaintAsActiveChanged() {
   StateChanged(state());
 }
 
+DEFINE_ENUM_CONVERTERS(
+    Button::ButtonState,
+    {Button::STATE_NORMAL, base::ASCIIToUTF16("STATE_NORMAL")},
+    {Button::STATE_HOVERED, base::ASCIIToUTF16("STATE_HOVERED")},
+    {Button::STATE_PRESSED, base::ASCIIToUTF16("STATE_PRESSED")},
+    {Button::STATE_DISABLED, base::ASCIIToUTF16("STATE_DISABLED")})
+
 BEGIN_METADATA(Button)
 METADATA_PARENT_CLASS(InkDropHostView)
+ADD_PROPERTY_METADATA(Button, ButtonState, State)
 END_METADATA()
 
 }  // namespace views

ui/views/controls/button/button.h

@@ -111,7 +111,10 @@ class VIEWS_EXPORT Button : public InkDropHostView,
   const base::string16& GetAccessibleName() const;
 
   // Get/sets the current display state of the button.
+  // TODO(pkasting): Replace all calls to state() with GetState(), and remove
+  // state().
   ButtonState state() const { return state_; }
+  ButtonState GetState() const;
   // Clients passing in STATE_DISABLED should consider calling
   // SetEnabled(false) instead because the enabled flag can affect other things
   // like event dispatching, focus traversals, etc. Calling SetEnabled(false)
@@ -182,6 +185,8 @@ class VIEWS_EXPORT Button : public InkDropHostView,
 
   void AddButtonObserver(ButtonObserver* observer);
   void RemoveButtonObserver(ButtonObserver* observer);
+  PropertyChangedSubscription AddStateChangedCallback(
+      PropertyChangedCallback callback);
 
   // Overridden from View:
   bool OnMousePressed(const ui::MouseEvent& event) override;
@@ -258,6 +263,7 @@ 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.

ui/views/controls/button/button_controller.cc

@@ -139,8 +139,6 @@ void ButtonController::OnGestureEvent(ui::GestureEvent* event) {
 
 void ButtonController::UpdateAccessibleNodeData(ui::AXNodeData* node_data) {}
 
-void ButtonController::OnStateChanged(Button::ButtonState old_state) {}
-
 bool ButtonController::IsTriggerableEvent(const ui::Event& event) {
   return event.type() == ui::ET_GESTURE_TAP_DOWN ||
          event.type() == ui::ET_GESTURE_TAP ||

ui/views/controls/button/button_controller.h

@@ -50,7 +50,6 @@ class VIEWS_EXPORT ButtonController {
   virtual void UpdateAccessibleNodeData(ui::AXNodeData* node_data);
 
   // Methods that parallel respective methods in Button:
-  virtual void OnStateChanged(Button::ButtonState old_state);
   virtual bool IsTriggerableEvent(const ui::Event& event);
 
  protected:

ui/views/controls/button/button_unittest.cc

@@ -895,21 +895,24 @@ TEST_F(ButtonTest, ChangingHighlightStateNotifiesListener) {
   EXPECT_FALSE(button_observer()->highlighted());
 }
 
-// Verifies that ButtonObserver is notified when the button state is changed,
-// and that the |observed_button| is passed to observer correctly.
+// Verifies that ButtonObserver is notified when the button state is changed.
 TEST_F(ButtonTest, ClickingButtonNotifiesObserverOfStateChanges) {
   CreateButtonWithObserver();
   EXPECT_FALSE(button_observer()->state_changed());
+  EXPECT_EQ(Button::STATE_NORMAL, button()->GetState());
 
   event_generator()->MoveMouseTo(button()->GetBoundsInScreen().CenterPoint());
   event_generator()->PressLeftButton();
   EXPECT_TRUE(button_observer()->state_changed());
+  EXPECT_EQ(Button::STATE_PRESSED, button()->GetState());
 
   button_observer()->Reset();
   EXPECT_FALSE(button_observer()->state_changed());
+  EXPECT_EQ(Button::STATE_PRESSED, button()->GetState());
 
   event_generator()->ReleaseLeftButton();
   EXPECT_TRUE(button_observer()->state_changed());
+  EXPECT_EQ(Button::STATE_HOVERED, button()->GetState());
 }
 
 // Verifies the ButtonObserver is notified whenever Button::SetState() is
@@ -917,15 +920,19 @@ TEST_F(ButtonTest, ClickingButtonNotifiesObserverOfStateChanges) {
 TEST_F(ButtonTest, SetStateNotifiesObserver) {
   CreateButtonWithObserver();
   EXPECT_FALSE(button_observer()->state_changed());
+  EXPECT_EQ(Button::STATE_NORMAL, button()->GetState());
 
   button()->SetState(Button::STATE_HOVERED);
   EXPECT_TRUE(button_observer()->state_changed());
+  EXPECT_EQ(Button::STATE_HOVERED, button()->GetState());
 
   button_observer()->Reset();
   EXPECT_FALSE(button_observer()->state_changed());
+  EXPECT_EQ(Button::STATE_HOVERED, button()->GetState());
 
   button()->SetState(Button::STATE_NORMAL);
   EXPECT_TRUE(button_observer()->state_changed());
+  EXPECT_EQ(Button::STATE_NORMAL, button()->GetState());
 }
 
 }  // namespace views

ui/views/controls/button/menu_button_controller.cc

@@ -180,21 +180,6 @@ void MenuButtonController::UpdateAccessibleNodeData(ui::AXNodeData* node_data) {
     node_data->SetDefaultActionVerb(ax::mojom::DefaultActionVerb::kOpen);
 }
 
-void MenuButtonController::OnStateChanged(LabelButton::ButtonState old_state) {
-  // State change occurs in IncrementPressedLocked() and
-  // DecrementPressedLocked().
-  if (pressed_lock_count_ != 0) {
-    // The button's state was changed while it was supposed to be locked in a
-    // pressed state. This shouldn't happen, but conceivably could if a caller
-    // tries to switch from enabled to disabled or vice versa while the button
-    // is pressed.
-    if (button()->state() == Button::STATE_NORMAL)
-      should_disable_after_press_ = false;
-    else if (button()->state() == Button::STATE_DISABLED)
-      should_disable_after_press_ = true;
-  }
-}
-
 bool MenuButtonController::IsTriggerableEvent(const ui::Event& event) {
   return ButtonController::IsTriggerableEvent(event) &&
          IsTriggerableEventType(event) && is_intentional_menu_trigger_;
@@ -308,7 +293,13 @@ void MenuButtonController::IncrementPressedLocked(
     const ui::LocatedEvent* event) {
   ++pressed_lock_count_;
   if (increment_pressed_lock_called_)
-    *(increment_pressed_lock_called_) = true;
+    *increment_pressed_lock_called_ = true;
+  if (!state_changed_subscription_) {
+    state_changed_subscription_ =
+        button()->AddStateChangedCallback(base::BindRepeating(
+            &MenuButtonController::OnButtonStateChangedWhilePressedLocked,
+            base::Unretained(this)));
+  }
   should_disable_after_press_ = button()->state() == Button::STATE_DISABLED;
   if (button()->state() != Button::STATE_PRESSED) {
     if (snap_ink_drop_to_activated)
@@ -327,6 +318,7 @@ void MenuButtonController::DecrementPressedLocked() {
   // If this was the last lock, manually reset state to the desired state.
   if (pressed_lock_count_ == 0) {
     menu_closed_time_ = TimeTicks::Now();
+    state_changed_subscription_.reset();
     LabelButton::ButtonState desired_state = Button::STATE_NORMAL;
     if (should_disable_after_press_) {
       desired_state = Button::STATE_DISABLED;
@@ -345,4 +337,15 @@ void MenuButtonController::DecrementPressedLocked() {
   }
 }
 
+void MenuButtonController::OnButtonStateChangedWhilePressedLocked() {
+  // The button's state was changed while it was supposed to be locked in a
+  // pressed state. This shouldn't happen, but conceivably could if a caller
+  // tries to switch from enabled to disabled or vice versa while the button is
+  // pressed.
+  if (button()->state() == Button::STATE_NORMAL)
+    should_disable_after_press_ = false;
+  else if (button()->state() == Button::STATE_DISABLED)
+    should_disable_after_press_ = true;
+}
+
 }  // namespace views

ui/views/controls/button/menu_button_controller.h

@@ -55,7 +55,6 @@ class VIEWS_EXPORT MenuButtonController : public ButtonController {
   bool OnKeyReleased(const ui::KeyEvent& event) override;
   void OnGestureEvent(ui::GestureEvent* event) override;
   void UpdateAccessibleNodeData(ui::AXNodeData* node_data) override;
-  void OnStateChanged(Button::ButtonState old_state) override;
   bool IsTriggerableEvent(const ui::Event& event) override;
 
   // Calls TakeLock with is_sibling_menu_show as false and a nullptr to the
@@ -88,6 +87,9 @@ class VIEWS_EXPORT MenuButtonController : public ButtonController {
 
   void DecrementPressedLocked();
 
+  // Called if the button state changes while pressed lock is engaged.
+  void OnButtonStateChangedWhilePressedLocked();
+
   // Our listener. Not owned.
   ButtonListener* const listener_;
 
@@ -113,6 +115,9 @@ class VIEWS_EXPORT MenuButtonController : public ButtonController {
   // we programmatically show a menu on a disabled button.
   bool should_disable_after_press_ = false;
 
+  // Subscribes to state changes on the button while pressed lock is engaged.
+  views::PropertyChangedSubscription state_changed_subscription_;
+
   base::WeakPtrFactory<MenuButtonController> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(MenuButtonController);