Refactoring to make adding ink drop animations easier

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 1225e67f5057482bc8db59bf65543a12a7f14603..cd46faa4535f5cfb08e2439131ef1fcd25a1a558 100644
--- a/ui/views/controls/button/menu_button.cc
+++ b/ui/views/controls/button/menu_button.cc
@@ -18,6 +18,7 @@
 #include "ui/gfx/text_constants.h"
 #include "ui/resources/grit/ui_resources.h"
 #include "ui/strings/grit/ui_strings.h"
+#include "ui/views/animation/ink_drop_delegate.h"
 #include "ui/views/controls/button/button.h"
 #include "ui/views/controls/button/menu_button_listener.h"
 #include "ui/views/mouse_constants.h"
@@ -126,6 +127,9 @@ bool MenuButton::Activate() {
     // We don't set our state here. It's handled in the MenuController code or
     // by our click listener.
 
+    InkDropDelegate* ink_drop_delegate = GetInkDropDelegate();

[bruthig, 2015/11/19 16:49:53]

Would it make things easier if GetInkDropDelegate() returned a dummy/stub
InkDropDelegate instead of null with no-op implementations for all methods?  We
wouldn't have to have 'if (ink_drop_delegate)' everywhere.

This was the intention of the current InkDropAnimationControllerStub class. 
(Which may not be needed anymore if we use a stub delegate instead.)

[varkha, 2015/11/19 23:34:35]

Done. May remove InkDropAnimationControllerStub later.

+    if (ink_drop_delegate)
+      ink_drop_delegate->OnAction(InkDropState::QUICK_ACTION);
     listener_->OnMenuButtonClicked(this, menu_position);
 
     if (destroyed) {
@@ -145,12 +149,6 @@ bool MenuButton::Activate() {
   return true;
 }
 
-void MenuButton::WillNotActivate() {
-  if (listener_) {
-    listener_->OnMenuButtonClickCanceled(this);
-  }
-}
-
 void MenuButton::OnPaint(gfx::Canvas* canvas) {
   LabelButton::OnPaint(canvas);
 
@@ -179,13 +177,20 @@ const char* MenuButton::GetClassName() const {
 }
 
 bool MenuButton::OnMousePressed(const ui::MouseEvent& event) {
+  if (event.IsOnlyLeftMouseButton()) {
+    InkDropDelegate* ink_drop_delegate = GetInkDropDelegate();
+    if (ink_drop_delegate)
+      ink_drop_delegate->OnAction(InkDropState::ACTION_PENDING);

[bruthig, 2015/11/19 16:49:53]

I don't think a MenuButton can know for certain that all mouse-pressed events
should show an ACTION_PENDING ripple. e.g. The
ChevronMenuButton::OnMenuButtonClicked() may close a menu on a mouse-press.  I'm
not sure how the ACTION_PENDING ripple would look in this case.

The underlying issue here is that I don't think a MenuButton has enough
information to know how to react to a mouse pressed event.

Also, it's odd that the ripple controlling logic is not the same as the logic
that calls Activate().  From what I can tell if a mouse press occurs in quicker
than |kMinimumMsBetweenButtonClicks| from when the menu was last shown, then the
ACTION_PENDING ripple is going to get shown even though no action will be
triggered.

[varkha, 2015/11/19 23:34:35]

Done.

+  }
+
   if (request_focus_on_press())
     RequestFocus();
   if (state() != STATE_DISABLED && ShouldEnterPushedState(event) &&
       HitTestPoint(event.location())) {
     TimeDelta delta = TimeTicks::Now() - menu_closed_time_;
-    if (delta.InMilliseconds() > kMinimumMsBetweenButtonClicks)
+    if (delta.InMilliseconds() > kMinimumMsBetweenButtonClicks) {
       return Activate();
+    }
   }
   return true;
 }
@@ -195,7 +200,9 @@ void MenuButton::OnMouseReleased(const ui::MouseEvent& event) {
       HitTestPoint(event.location()) && !InDrag()) {
     Activate();
   } else {
-    WillNotActivate();
+    InkDropDelegate* ink_drop_delegate = GetInkDropDelegate();
+    if (ink_drop_delegate)
+      ink_drop_delegate->OnAction(InkDropState::HIDDEN);

[bruthig, 2015/11/19 16:49:53]

Somewhat similar concerns here to the ones in OnMousePressed.  How can we be
sure that all mouse release events should hide the ripple?
Looking forward, once jonross@'s work to make menu's asynchronous, I'm assuming
we will be getting the mouse release events when the menu is shown.  In which
case the ripple should be in the ACTIVATED state, but this logic would cause it
to be hidden.

[varkha, 2015/11/19 23:34:35]

Will need to rethink what happens in asynchronous case.

     LabelButton::OnMouseReleased(event);
   }
 }