Converted all Views to use an InkDropDelegate instead of a InkDropAnimationController.

chrome/browser/ui/views/bar_control_button.cc

Index: chrome/browser/ui/views/bar_control_button.cc
diff --git a/chrome/browser/ui/views/bar_control_button.cc b/chrome/browser/ui/views/bar_control_button.cc
index 6eb966be892f383899074e4fa5e3c05ea4e0a2e5..bc23f5d0cb0f4751a999ed6f4d88023bd41d2eb6 100644
--- a/chrome/browser/ui/views/bar_control_button.cc
+++ b/chrome/browser/ui/views/bar_control_button.cc
@@ -7,8 +7,7 @@
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/gfx/vector_icons_public.h"
-#include "ui/views/animation/ink_drop_animation_controller.h"
-#include "ui/views/animation/ink_drop_animation_controller_factory.h"
+#include "ui/views/animation/button_ink_drop_delegate.h"
 #include "ui/views/border.h"
 
 namespace {
@@ -21,21 +20,23 @@ const int kButtonExtraTouchSize = 4;
 BarControlButton::BarControlButton(views::ButtonListener* listener)
     : views::ImageButton(listener),
       id_(gfx::VectorIconId::VECTOR_ICON_NONE),
-      ink_drop_animation_controller_(
-          views::InkDropAnimationControllerFactory::
-              CreateInkDropAnimationController(this)) {
+      ink_drop_delegate_(new views::ButtonInkDropDelegate(this, this)) {
+  set_ink_drop_delegate(ink_drop_delegate_.get());
+  set_has_ink_drop_action_on_click(true);
+
   const int kInkDropLargeSize = 32;
   const int kInkDropLargeCornerRadius = 4;
   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);
 }
 
-BarControlButton::~BarControlButton() {}
+BarControlButton::~BarControlButton() {
+  set_ink_drop_delegate(nullptr);
+  ink_drop_delegate_.reset();

[Peter Kasting, 2015/12/12 01:56:18]

I don't think you need to explicitly do this.  It's about to be destroyed
anyway, and that destruction will happen before the base class destructor runs. 
I don't think this explicit destruction affects anything.

I'm also a bit skeptical about the set_ink_drop_delegate(nullptr) call too.  The
only purpose of this call is to make the DCHECK in the base class work.  But
that DCHECK only checks that this set call has been made, and importantly, it
_doesn't_ actually check that the ink drop delegate has been destroyed, which is
the critical bit.  So to me this is all theater, and we should remove both the
set call here and the DCHECK there.  It doesn't actually make the code more
correct or help other subclassers write safer code.

There _are_ ways we could guarantee the destruction of this object, but I think
they're more mess/work than they're worth; one is to give the base class the
address of the scoped_ptr instead of the contained raw pointer, and the other is
to hook up some delegate callbacks so when the delegate is destroyed it
notifiers an interested listener.  There's no benefit to these other than
sanity-checking the destruction, and given that that destruction should happen
correctly with no explicit code written in the subclass destructor at all, I
wouldn't do them.

[varkha, 2015/12/12 02:43:03]

I partially agree with this comment. Setting the delegate pointer in the base
class here is indeed to verify that nothing accesses it in the destructor of the
base class. Resetting the scoped_ptr here however is logical as this
BarControlButton is an InkDropHost and so its RemoveInkDropLayer will get called
when ink_drop_delegate_ will be destroyed. Doing it after dtor returns seemed a
bit wrong to me thus this sequence.
I agree with the rest - there are ways to enforce correct sequence but they all
seem like an overkill here. Some are mentioned in review for
https://codereview.chromium.org/1494433003/. My original approach there was to
own the delegate in the base class and make the DCHECK verify that it has been
disposed of before the base class destructor but I have changed that in the last
patch set following a comment.
If I may suggest here I would make the code consistent everywhere as a first
step and if we see that this destruction sequence can be consolidated refactor
all those paths in a separate CL.

[Peter Kasting, 2015/12/12 02:46:09]

Eh?  Members are destroyed before the dtor returns, not after.  Resetting the
scoped_ptr here explicitly has basically no observable effect at all.  (It just
affects the relative order of different members' destruction, it does not affect
when members are destroyed relative to when the subclass portion of the object
is destroyed or relative to when the destructor returns.)

+}
 
 void BarControlButton::SetIcon(
     gfx::VectorIconId id,
@@ -63,12 +64,6 @@ void BarControlButton::OnNativeThemeChanged(const ui::NativeTheme* theme) {
   OnThemeChanged();
 }
 
-void BarControlButton::Layout() {
-  ImageButton::Layout();
-
-  ink_drop_animation_controller_->SetInkDropCenter(CalculateInkDropCenter());
-}
-
 void BarControlButton::AddInkDropLayer(ui::Layer* ink_drop_layer) {
   // TODO(estade|tdanderson): The ink drop layer should be positioned behind
   //                          the button's image.
@@ -89,54 +84,8 @@ gfx::Point BarControlButton::CalculateInkDropCenter() const {
 }
 
 bool BarControlButton::OnMousePressed(const ui::MouseEvent& event) {
-  if (IsTriggerableEvent(event)) {
-    ink_drop_animation_controller_->AnimateToState(
-        views::InkDropState::ACTION_PENDING);
-  }
+  if (IsTriggerableEvent(event))
+    ink_drop_delegate()->OnAction(views::InkDropState::ACTION_PENDING);
 
   return ImageButton::OnMousePressed(event);
 }
-
-void BarControlButton::OnGestureEvent(ui::GestureEvent* 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_TAP:
-      ink_drop_state = views::InkDropState::QUICK_ACTION;
-      break;
-    case ui::ET_GESTURE_LONG_TAP:
-      ink_drop_state = views::InkDropState::SLOW_ACTION;
-      break;
-    case ui::ET_GESTURE_END:
-    case ui::ET_GESTURE_TAP_CANCEL:
-      ink_drop_state = views::InkDropState::HIDDEN;
-      break;
-    default:
-      return;
-  }
-  ink_drop_animation_controller_->AnimateToState(ink_drop_state);
-
-  ImageButton::OnGestureEvent(event);
-}
-
-void BarControlButton::OnMouseReleased(const ui::MouseEvent& event) {
-  if (!HitTestPoint(event.location()))
-    ink_drop_animation_controller_->AnimateToState(views::InkDropState::HIDDEN);
-
-  ImageButton::OnMouseReleased(event);
-}
-
-void BarControlButton::NotifyClick(const ui::Event& event) {
-  ink_drop_animation_controller_->AnimateToState(
-      views::InkDropState::QUICK_ACTION);
-
-  ImageButton::NotifyClick(event);
-}