Unify MenuButton Pushed state logic

ui/views/controls/button/menu_button.cc

Index: ui/views/controls/button/menu_button.cc
diff --git a/ui/views/controls/button/menu_button.cc b/ui/views/controls/button/menu_button.cc
index debd227b802b2e43c2bc4a735be6e1d1a002ad2e..b3656d072d6fad39dc336b4f7e5a469a7f98f4d1 100644
--- a/ui/views/controls/button/menu_button.cc
+++ b/ui/views/controls/button/menu_button.cc
@@ -171,51 +171,41 @@ const char* MenuButton::GetClassName() const {
 
 bool MenuButton::OnMousePressed(const ui::MouseEvent& event) {
   RequestFocus();
-  if (state() != STATE_DISABLED) {
-    // If we're draggable (GetDragOperations returns a non-zero value), then
-    // don't pop on press, instead wait for release.
-    if (event.IsOnlyLeftMouseButton() &&
-        HitTestPoint(event.location()) &&
-        GetDragOperations(event.location()) == ui::DragDropTypes::DRAG_NONE) {
-      TimeDelta delta = TimeTicks::Now() - menu_closed_time_;
-      if (delta.InMilliseconds() > kMinimumMsBetweenButtonClicks)
-        return Activate();
-    }
+  if (state() != STATE_DISABLED && ShouldEnterPushedState(event) &&
+      HitTestPoint(event.location())) {
+    TimeDelta delta = TimeTicks::Now() - menu_closed_time_;
+    if (delta.InMilliseconds() > kMinimumMsBetweenButtonClicks)
+      return Activate();
   }
   return true;
 }
 
 void MenuButton::OnMouseReleased(const ui::MouseEvent& event) {
-  // Explicitly test for left mouse button to show the menu. If we tested for
-  // !IsTriggerableEvent it could lead to a situation where we end up showing
-  // the menu and context menu (this would happen if the right button is not
-  // triggerable and there's a context menu).
-  if (GetDragOperations(event.location()) != ui::DragDropTypes::DRAG_NONE &&
-      state() != STATE_DISABLED && !InDrag() && event.IsOnlyLeftMouseButton() &&
-      HitTestPoint(event.location())) {
+  if (state() == STATE_DISABLED)
+    return;
+  if (ShouldEnterPushedState(event) && HitTestPoint(event.location()))
     Activate();
-  } else {
+  else
     LabelButton::OnMouseReleased(event);
-  }
 }
 
 void MenuButton::OnMouseEntered(const ui::MouseEvent& event) {
   if (pressed_lock_count_ == 0)  // Ignore mouse movement if state is locked.
-    CustomButton::OnMouseEntered(event);
+    LabelButton::OnMouseEntered(event);
 }
 
 void MenuButton::OnMouseExited(const ui::MouseEvent& event) {
   if (pressed_lock_count_ == 0)  // Ignore mouse movement if state is locked.
-    CustomButton::OnMouseExited(event);
+    LabelButton::OnMouseExited(event);
 }
 
 void MenuButton::OnMouseMoved(const ui::MouseEvent& event) {
   if (pressed_lock_count_ == 0)  // Ignore mouse movement if state is locked.
-    CustomButton::OnMouseMoved(event);
+    LabelButton::OnMouseMoved(event);
 }
 
 void MenuButton::OnGestureEvent(ui::GestureEvent* event) {
-  if (state() != STATE_DISABLED && event->type() == ui::ET_GESTURE_TAP &&
+  if (state() != STATE_DISABLED && ShouldEnterPushedState(*event) &&
       !Activate()) {
     // When |Activate()| returns |false|, it means that a menu is shown and
     // has handled the gesture event. So, there is no need to further process
@@ -287,6 +277,32 @@ gfx::Rect MenuButton::GetChildAreaBounds() {
   return gfx::Rect(s);
 }
 
+bool MenuButton::ShouldEnterPushedState(const ui::Event& event) {
+  if (event.IsMouseEvent()) {
+    const ui::MouseEvent& mouseev = static_cast<const ui::MouseEvent&>(event);
+    // Active on left mouse button only, to prevent a menu from being activated
+    // when a right-click would also activate a context menu.
+    if (!mouseev.IsOnlyLeftMouseButton())
+      return false;
+    // Activate on press if dragging is not supported.
+    if (event.type() == ui::ET_MOUSE_PRESSED &&
+        GetDragOperations(mouseev.location()) == ui::DragDropTypes::DRAG_NONE) {

[flackr, 2014/10/14 14:11:06]

nit: I think this would look cleaner if you had EventType active_on =
GetDragOperations(mouseev.location()) == ui::DragDropTypes::DRAG_NONE ?
ui::ET_MOUSE_PRESSED : ui::ET_MOUSE_RELEASED.

Then you can have a single condition which checks for the matching event type
and doesn't have to potentially call GetDragOperations twice.

[jonross, 2014/10/14 16:00:48]

Done.

+      return true;
+    }
+    // Activate on release if dragging is supported and a drag is not in
+    // progress.
+    if (event.type() == ui::ET_MOUSE_RELEASED && !InDrag() &&
+        GetDragOperations(mouseev.location()) != ui::DragDropTypes::DRAG_NONE) {
+      return true;
+    }
+  }
+
+  if (event.type() == ui::ET_GESTURE_TAP)
+    return true;
+
+  return false;
+}
+
 void MenuButton::IncrementPressedLocked() {
   ++pressed_lock_count_;
   SetState(STATE_PRESSED);