Ink drops with touch input are redundant on Windows since the OS does something similar

ui/views/animation/ink_drop_host_view.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/animation/ink_drop_host_view.h"
 
 #include "ui/events/event.h"
 #include "ui/events/scoped_target_handler.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/geometry/size_conversions.h"
@@ skipping 154 matching lines @@
 }
 
 gfx::Point InkDropHostView::GetInkDropCenterBasedOnLastEvent() const {
   return last_ripple_triggering_event_
              ? last_ripple_triggering_event_->location()
              : GetLocalBounds().CenterPoint();
 }
 
 void InkDropHostView::AnimateInkDrop(InkDropState state,
                                      const ui::LocatedEvent* event) {
+#if defined(OS_WIN)
+  // On Windows, don't initiate ink-drops. Additionally, certain event states
+  // should dismiss existing ink-drop animations.
+  if (event && event->IsGestureEvent()) {

[bruthig, 2016/06/30 00:23:11]

This would definitely benefit from some tests.

[kylix_rd, 2016/06/30 17:31:25]

I presume you're referring to the whole block and not just the above test?

[bruthig, 2016/06/30 17:35:36]

Correct, but at least the number of test cases required should be reduced with
the simplified logic.

+    // Don't transition the ink-drop to a "pending" state from a touch event.
+    if (state == InkDropState::ACTION_PENDING ||
+        state == InkDropState::ALTERNATE_ACTION_PENDING)
+      return;
+    // If the state is already pending, presumably from a mouse or keyboard
+    // event, then a "triggered" action state should be allowed. Conversely,
+    // if the state had already transitioned to hidden, then the touch event
+    // is ignored.
+    if ((state == InkDropState::ACTION_TRIGGERED ||
+         state == InkDropState::ALTERNATE_ACTION_TRIGGERED) &&
+        ink_drop_->GetTargetInkDropState() == InkDropState::HIDDEN)

[bruthig, 2016/06/30 00:23:11]

What is the use-case for the "&& HIDDEN" clause here?  Should it be a '!='
instead of an '=='?

After thinking about this a bit more, does the following logic simplify things? 
Essentially we would only prevent ripples from transitioning from HIDDEN to
visible on gesture events, unless it is an ACTIVATED state.

if (event &&
    event->IsGestureEvent() &&
    ink_drop_->GetTargetInkDropState() == HIDDEN &&
    state != ACTIVATED) {
  return;
}

[kylix_rd, 2016/06/30 13:39:37]

The idea here is that a triggered action from a touch event should only animate
if the state is PENDING. If the current state is HIDDEN, then don't show the
TRIGGERED animation.
What about DEACTIVATED? Would you not still want to allow that through as well?
At least according to the comments in ink_drop_state.h.

[bruthig, 2016/06/30 17:21:49]

Ah right, makes sense.  I must have been tired.
The DEACTIVATED animations should all make it through because the
ink_drop_->GetTargetInkDropState() should never be HIDDEN when a request for
DEACTIVED happens. i.e. the DEACTIVATED state should be preceded by an ACTIVATED
state and the ACTIVATED animations are shown for gestures.  Even if we somehow
got a request to show the DEACTIVATED animation when GetTargetInkDropState() is
HIDDEN, I think it would be reasonable to not show it.

[kylix_rd, 2016/06/30 17:31:25]

Aside from this logic being a little more complicated than necessary, are there
any cases not taken into account?

Either way, I can create a new CL with your suggestions.

[bruthig, 2016/06/30 17:35:36]

Nothing specific to this CL that I can think of.  There are still problems with
multiple input sources interacting with a View but that is definitely outside
the scope of this change.
Awesome, thanks :)  Sorry I wasn't more on the ball earlier.

+      return;
+  }
+#endif
   last_ripple_triggering_event_.reset(
       event ? ui::Event::Clone(*event).release()->AsLocatedEvent() : nullptr);
   ink_drop_->AnimateToState(state);
 }
 
 void InkDropHostView::VisibilityChanged(View* starting_from, bool is_visible) {
   View::VisibilityChanged(starting_from, is_visible);
   if (GetWidget() && !is_visible) {
     ink_drop()->AnimateToState(InkDropState::HIDDEN);
     ink_drop()->SetHovered(false);
@@ skipping 39 matching lines @@
   if (has_an_ink_drop) {
     ink_drop_.reset(new InkDropImpl(this));
     gesture_handler_.reset(new InkDropGestureHandler(this, ink_drop_.get()));
   } else {
     gesture_handler_.reset();
     ink_drop_.reset(new InkDropStub());
   }
 }
 
 }  // namespace views