Refactoring to make adding ink drop animations easier

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

Index: chrome/browser/ui/views/toolbar/toolbar_action_view.cc
diff --git a/chrome/browser/ui/views/toolbar/toolbar_action_view.cc b/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
index f2e5a4232ff539e9e490e82a63f8a2526326ddb5..12f7a1870618768fb2679930877d94f8d7535225 100644
--- a/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
+++ b/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
@@ -26,8 +26,7 @@
 #include "ui/gfx/image/image_skia_operations.h"
 #include "ui/gfx/image/image_skia_source.h"
 #include "ui/resources/grit/ui_resources.h"
-#include "ui/views/animation/ink_drop_animation_controller.h"
-#include "ui/views/animation/ink_drop_animation_controller_factory.h"
+#include "ui/views/animation/toolbar_ink_drop_delegate.h"
 #include "ui/views/controls/button/label_button_border.h"
 #include "ui/views/controls/menu/menu_controller.h"
 #include "ui/views/controls/menu/menu_model_adapter.h"
@@ -68,9 +67,7 @@ ToolbarActionView::ToolbarActionView(
       called_register_command_(false),
       wants_to_run_(false),
       menu_(nullptr),
-      ink_drop_animation_controller_(
-          views::InkDropAnimationControllerFactory::
-              CreateInkDropAnimationController(this)),
+      ink_drop_delegate_(new views::ToolbarInkDropDelegate(this)),
       weak_factory_(this) {
   set_id(VIEW_ID_BROWSER_ACTION);
   view_controller_->SetDelegate(this);
@@ -83,11 +80,8 @@ ToolbarActionView::ToolbarActionView(
   const int kInkDropLargeCornerRadius = 5;
   const int kInkDropSmallSize = 24;
   const int kInkDropSmallCornerRadius = 2;
-
-  ink_drop_animation_controller_->SetInkDropSize(
-      gfx::Size(kInkDropLargeSize, kInkDropLargeSize),
-      kInkDropLargeCornerRadius,
-      gfx::Size(kInkDropSmallSize, kInkDropSmallSize),
+  ink_drop_delegate_->SetInkDropSize(
+      kInkDropLargeSize, kInkDropLargeCornerRadius, kInkDropSmallSize,
       kInkDropSmallCornerRadius);
 
   // We also listen for browser theme changes on linux because a switch from or
@@ -136,25 +130,15 @@ gfx::Point ToolbarActionView::CalculateInkDropCenter() const {
   return GetLocalBounds().CenterPoint();
 }
 
+views::InkDropDelegate* ToolbarActionView::GetInkDropDelegate() const {
+  return ink_drop_delegate_.get();
+}
+
 gfx::Size ToolbarActionView::GetPreferredSize() const {
   return gfx::Size(ToolbarActionsBar::IconWidth(false),
                    ToolbarActionsBar::IconHeight());
 }
 
-void ToolbarActionView::Layout() {
-  MenuButton::Layout();
-  ink_drop_animation_controller_->SetInkDropCenter(CalculateInkDropCenter());
-}
-
-bool ToolbarActionView::OnMousePressed(const ui::MouseEvent& event) {
-  // views::MenuButton actions are only triggered by left mouse clicks.
-  if (event.IsLeftMouseButton()) {
-    ink_drop_animation_controller_->AnimateToState(
-        views::InkDropState::ACTION_PENDING);
-  }
-  return MenuButton::OnMousePressed(event);
-}
-
 void ToolbarActionView::OnGestureEvent(ui::GestureEvent* event) {
   // TODO(varkha): refactor similar implementations in ToolbarButton,
   // FindBarButton and here.
@@ -163,48 +147,15 @@ void ToolbarActionView::OnGestureEvent(ui::GestureEvent* event) {
     event->StopPropagation();
     return;
   }
-
-  MenuButton::OnGestureEvent(event);
-
-  views::InkDropState ink_drop_state = views::InkDropState::HIDDEN;
-  switch (event->type()) {
-    case ui::ET_GESTURE_TAP_DOWN:
-      ink_drop_state = views::InkDropState::ACTION_PENDING;
-      // The ui::ET_GESTURE_TAP_DOWN event needs to be marked as handled so that
-      // subsequent events for the gesture are sent to |this|.
-      event->SetHandled();
-      break;
-    case ui::ET_GESTURE_LONG_PRESS:
-      ink_drop_state = views::InkDropState::SLOW_ACTION_PENDING;
-      break;
-    case ui::ET_GESTURE_LONG_TAP:
-      ink_drop_state = views::InkDropState::SLOW_ACTION;
-      break;
-    case ui::ET_GESTURE_SCROLL_BEGIN:
-    case ui::ET_GESTURE_END:
-      ink_drop_state = views::InkDropState::HIDDEN;
-      break;
-    default:
-      return;
-  }
-
-  views::InkDropState current_ink_drop_state =
-      ink_drop_animation_controller_->GetInkDropState();
-
-  if (ink_drop_state == views::InkDropState::HIDDEN &&
-      (current_ink_drop_state == views::InkDropState::QUICK_ACTION ||
-       current_ink_drop_state == views::InkDropState::SLOW_ACTION ||
-       current_ink_drop_state == views::InkDropState::DEACTIVATED)) {
-    // These InkDropStates automatically transition to the HIDDEN state so we
-    // don't make an explicit call. Explicitly animating to HIDDEN in this case
-    // would prematurely pre-empt these animations.
-    return;
+  if (event->type() == ui::ET_GESTURE_TAP_DOWN) {

[bruthig, 2015/11/12 16:41:15]

This is set to handled for ink drop purposes only correct? I would think it
should be moved to the ToolbarInkDropDelegate.

[varkha, 2015/11/13 00:32:41]

Done.

+    // The ui::ET_GESTURE_TAP_DOWN event needs to be marked as handled so that
+    // subsequent events for the gesture are sent to |this|.
+    event->SetHandled();
   }
-  ink_drop_animation_controller_->AnimateToState(ink_drop_state);
+  MenuButton::OnGestureEvent(event);
 }
 
 void ToolbarActionView::OnDragDone() {
-  ink_drop_animation_controller_->AnimateToState(views::InkDropState::HIDDEN);
   views::MenuButton::OnDragDone();
   delegate_->OnToolbarActionViewDragDone();
 }
@@ -233,16 +184,10 @@ void ToolbarActionView::OnMenuButtonClicked(views::View* sender,
     context_menu_controller()->ShowContextMenuForView(this, point,
                                                       ui::MENU_SOURCE_NONE);
   } else {
-    ink_drop_animation_controller_->AnimateToState(
-        views::InkDropState::QUICK_ACTION);
     view_controller_->ExecuteAction(true);
   }
 }
 
-void ToolbarActionView::OnMenuButtonClickCanceled(views::View* sender) {
-  ink_drop_animation_controller_->AnimateToState(views::InkDropState::HIDDEN);
-}
-
 void ToolbarActionView::UpdateState() {
   content::WebContents* web_contents = GetCurrentWebContents();
   if (SessionTabHelper::IdForTab(web_contents) < 0)
@@ -401,8 +346,9 @@ void ToolbarActionView::DoShowContextMenu(
       delegate_->GetOverflowReferenceView()->GetWidget() :
       GetWidget();
 
-  ink_drop_animation_controller_->AnimateToState(
-      views::InkDropState::ACTIVATED);
+  views::InkDropDelegate* ink_drop_delegate = GetInkDropDelegate();
+  if (ink_drop_delegate)
+    ink_drop_delegate->OnActivated();

[bruthig, 2015/11/12 16:41:15]

Is there any benefit to having the Views invoke state changes on the
InkDropDelegate and having it proxy the call off to the
InkDropAnimationController instead of just calling
InkDropAnimationController::AnimateToState() directly?

The disadvantage I see with the current implementation is Views might eventually
want access to some InkDropController specific data, i.e. InkDropState, and I
don't think having to reach through the InkDropDelegate, or proxying the calls
is the best choice here.

[varkha, 2015/11/13 00:32:41]

I wanted to hide more of the InkDropAnimationcontroller from the clients in case
it needs to do more. I was hoping that what is now called InkDropDelegate would
be able to query and use view's state if necessary.

 
   views::MenuModelAdapter adapter(context_menu_model);
   menu_ = adapter.CreateMenu();
@@ -416,8 +362,8 @@ void ToolbarActionView::DoShowContextMenu(
     menu_ = nullptr;
     return;
   }
-  ink_drop_animation_controller_->AnimateToState(
-      views::InkDropState::DEACTIVATED);
+  if (ink_drop_delegate)
+    ink_drop_delegate->OnDeactivated();
 
   menu_runner_.reset();
   menu_ = nullptr;