Correctly preventDefault pinch zoom

Pinch zoom was incorrectly not being preventDefault'd sometimes, in two
different scenarios: either the transition from scroll->pinch, or from
fling->pinch. In both cases, events were being queued in the
TouchpadPinchEventQueue, then TouchpadPinchEventQueue::ProcessMouseWheelAck
was receiving ACKs, which TPEQ assumed were the ACKs for the events that it
had already queued. While this is often true, it was not always the case.
Sometimes, the event ACKs from the scroll or fling would arrive after the
pinch events had been queued already, in which case the ACKs would be
processed as if they were for pinch events, when they weren't. This
resulted in the pinch zoom not always being preventDefault'd.

This CL fixes this by checking the event that was being ACK'd vs the event
that TPEQ expects an ACK for, and if they don't match then it early outs
and ignores the ACK.

Bug: 908758
Change-Id: Ifbad8e3ae46947c75b4680dc516398b69cf2cc89
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1725349
Commit-Queue: Mario Bianucci <mabian@microsoft.com>
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688161}

content/browser/renderer_host/input/input_router_impl.cc

@@ -664,9 +664,8 @@ void InputRouterImpl::MouseWheelEventHandled(
   if (overscroll)
     DidOverscroll(overscroll.value());
 
-  wheel_event_queue_.ProcessMouseWheelAck(source, state, event.latency);
-  touchpad_pinch_event_queue_.ProcessMouseWheelAck(source, state,
-                                                   event.latency);
+  wheel_event_queue_.ProcessMouseWheelAck(source, state, event);
+  touchpad_pinch_event_queue_.ProcessMouseWheelAck(source, state, event);
 }
 
 void InputRouterImpl::OnHasTouchEventHandlers(bool has_handlers) {

content/browser/renderer_host/input/mouse_wheel_event_queue.cc

@@ -5,7 +5,6 @@
 #include "content/browser/renderer_host/input/mouse_wheel_event_queue.h"
 
 #include "base/metrics/histogram_macros.h"
-#include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
 #include "content/common/input/input_event_dispatch_type.h"
 #include "content/common/input/web_mouse_wheel_event_traits.h"
@@ -20,23 +19,6 @@ using ui::LatencyInfo;
 
 namespace content {
 
-// This class represents a single queued mouse wheel event. Its main use
-// is that it is reported via trace events.
-class QueuedWebMouseWheelEvent : public MouseWheelEventWithLatencyInfo {
- public:
-  QueuedWebMouseWheelEvent(const MouseWheelEventWithLatencyInfo& original_event)
-      : MouseWheelEventWithLatencyInfo(original_event) {
-    TRACE_EVENT_ASYNC_BEGIN0("input", "MouseWheelEventQueue::QueueEvent", this);
-  }
-
-  ~QueuedWebMouseWheelEvent() {
-    TRACE_EVENT_ASYNC_END0("input", "MouseWheelEventQueue::QueueEvent", this);
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(QueuedWebMouseWheelEvent);
-};
-
 MouseWheelEventQueue::MouseWheelEventQueue(MouseWheelEventQueueClient* client)
     : client_(client),
       send_wheel_events_async_(false),
@@ -125,12 +107,21 @@ bool MouseWheelEventQueue::CanGenerateGestureScroll(
 void MouseWheelEventQueue::ProcessMouseWheelAck(
     InputEventAckSource ack_source,
     InputEventAckState ack_result,
-    const LatencyInfo& latency_info) {
+    const MouseWheelEventWithLatencyInfo& ack_event) {
   TRACE_EVENT0("input", "MouseWheelEventQueue::ProcessMouseWheelAck");
   if (!event_sent_for_gesture_ack_)
     return;
 
-  event_sent_for_gesture_ack_->latency.AddNewLatencyFrom(latency_info);
+  // |ack_event.event| should be the same as
+  // |event_sent_for_gesture_ack_->event|. If they aren't, then don't continue
+  // processing the ack. The two events can potentially be different because
+  // TouchpadPinchEventQueue also dispatches wheel events, and any wheel event
+  // ack that is received is sent to both *EventQueue::ProcessMouseWheelAck
+  // methods.
+  if (ack_event.event != event_sent_for_gesture_ack_->event)
+    return;
+
+  event_sent_for_gesture_ack_->latency.AddNewLatencyFrom(ack_event.latency);
   client_->OnMouseWheelEventAck(*event_sent_for_gesture_ack_, ack_source,
                                 ack_result);
 

content/browser/renderer_host/input/mouse_wheel_event_queue.h

@@ -9,6 +9,7 @@
 
 #include "base/containers/circular_deque.h"
 #include "base/time/time.h"
+#include "base/trace_event/trace_event.h"
 #include "content/browser/renderer_host/event_with_latency_info.h"
 #include "content/common/content_export.h"
 #include "content/public/common/input_event_ack_source.h"
@@ -17,7 +18,22 @@
 
 namespace content {
 
-class QueuedWebMouseWheelEvent;
+// This class represents a single queued mouse wheel event. Its main use
+// is that it is reported via trace events.
+class QueuedWebMouseWheelEvent : public MouseWheelEventWithLatencyInfo {
+ public:
+  QueuedWebMouseWheelEvent(const MouseWheelEventWithLatencyInfo& original_event)
+      : MouseWheelEventWithLatencyInfo(original_event) {
+    TRACE_EVENT_ASYNC_BEGIN0("input", "MouseWheelEventQueue::QueueEvent", this);
+  }
+
+  ~QueuedWebMouseWheelEvent() {
+    TRACE_EVENT_ASYNC_END0("input", "MouseWheelEventQueue::QueueEvent", this);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(QueuedWebMouseWheelEvent);
+};
 
 // Interface with which MouseWheelEventQueue can forward mouse wheel events,
 // and dispatch mouse wheel event responses.
@@ -57,7 +73,7 @@ class CONTENT_EXPORT MouseWheelEventQueue {
   // renderer.
   void ProcessMouseWheelAck(InputEventAckSource ack_source,
                             InputEventAckState ack_result,
-                            const ui::LatencyInfo& latency_info);
+                            const MouseWheelEventWithLatencyInfo& ack_event);
 
   // When GestureScrollBegin is received, and it is a different source
   // than mouse wheels terminate the current GestureScroll if there is one.
@@ -74,6 +90,10 @@ class CONTENT_EXPORT MouseWheelEventQueue {
     return event_sent_for_gesture_ack_ != nullptr;
   }
 
+  blink::WebMouseWheelEvent get_wheel_event_awaiting_ack_for_testing() {
+    return event_sent_for_gesture_ack_->event;
+  }
+
  private:
   void TryForwardNextEventToRenderer();
   void SendScrollEnd(blink::WebGestureEvent update_event, bool synthetic);

content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc

@@ -221,8 +221,10 @@ class MouseWheelEventQueueTest : public testing::Test,
   }
 
   void SendMouseWheelEventAck(InputEventAckState ack_result) {
+    const MouseWheelEventWithLatencyInfo mouse_event_with_latency_info(
+        queue_->get_wheel_event_awaiting_ack_for_testing(), ui::LatencyInfo());
     queue_->ProcessMouseWheelAck(InputEventAckSource::COMPOSITOR_THREAD,
-                                 ack_result, ui::LatencyInfo());
+                                 ack_result, mouse_event_with_latency_info);
   }
   void SendMouseWheel(float x,
                       float y,

content/browser/renderer_host/input/touchpad_pinch_event_queue.cc

@@ -126,21 +126,30 @@ void TouchpadPinchEventQueue::QueueEvent(
 void TouchpadPinchEventQueue::ProcessMouseWheelAck(
     InputEventAckSource ack_source,
     InputEventAckState ack_result,
-    const ui::LatencyInfo& latency_info) {
+    const MouseWheelEventWithLatencyInfo& ack_event) {
   TRACE_EVENT0("input", "TouchpadPinchEventQueue::ProcessMouseWheelAck");
   if (!pinch_event_awaiting_ack_)
     return;
 
+  // |ack_event.event| should be the same as the wheel_event_awaiting_ack_. If
+  // they aren't, then don't continue processing the ack. The two events can
+  // potentially be different because MouseWheelEventQueue also dispatches wheel
+  // events, and any wheel event ack that is received is sent to both
+  // *EventQueue::ProcessMouseWheelAck methods.
+  if (wheel_event_awaiting_ack_ != ack_event.event)
+    return;
+
   if (pinch_event_awaiting_ack_->event.GetType() ==
           blink::WebInputEvent::kGesturePinchUpdate &&
       !first_event_prevented_.has_value())
     first_event_prevented_ = (ack_result == INPUT_EVENT_ACK_STATE_CONSUMED);
 
-  pinch_event_awaiting_ack_->latency.AddNewLatencyFrom(latency_info);
+  pinch_event_awaiting_ack_->latency.AddNewLatencyFrom(ack_event.latency);
   client_->OnGestureEventForPinchAck(*pinch_event_awaiting_ack_, ack_source,
                                      ack_result);
 
   pinch_event_awaiting_ack_.reset();
+  wheel_event_awaiting_ack_.reset();
   TryForwardNextEventToRenderer();
 }
 
@@ -192,10 +201,10 @@ void TouchpadPinchEventQueue::TryForwardNextEventToRenderer() {
     }
   }
 
+  wheel_event_awaiting_ack_ = CreateSyntheticWheelFromTouchpadPinchEvent(
+      pinch_event_awaiting_ack_->event, phase, cancelable);
   const MouseWheelEventWithLatencyInfo synthetic_wheel(
-      CreateSyntheticWheelFromTouchpadPinchEvent(
-          pinch_event_awaiting_ack_->event, phase, cancelable),
-      pinch_event_awaiting_ack_->latency);
+      wheel_event_awaiting_ack_.value(), pinch_event_awaiting_ack_->latency);
 
   client_->SendMouseWheelEventForPinchImmediately(synthetic_wheel);
 }

content/browser/renderer_host/input/touchpad_pinch_event_queue.h

@@ -15,10 +15,6 @@
 #include "content/public/common/input_event_ack_state.h"
 #include "third_party/blink/public/platform/web_input_event.h"
 
-namespace ui {
-class LatencyInfo;
-}  // namespace ui
-
 namespace content {
 
 class QueuedTouchpadPinchEvent;
@@ -57,10 +53,14 @@ class CONTENT_EXPORT TouchpadPinchEventQueue {
   // by the renderer.
   void ProcessMouseWheelAck(InputEventAckSource ack_source,
                             InputEventAckState ack_result,
-                            const ui::LatencyInfo& latency_info);
+                            const MouseWheelEventWithLatencyInfo& ack_event);
 
   bool has_pending() const WARN_UNUSED_RESULT;
 
+  blink::WebMouseWheelEvent get_wheel_event_awaiting_ack_for_testing() {
+    return wheel_event_awaiting_ack_.value();
+  }
+
  private:
   void TryForwardNextEventToRenderer();
 
@@ -69,6 +69,7 @@ class CONTENT_EXPORT TouchpadPinchEventQueue {
 
   base::circular_deque<std::unique_ptr<QueuedTouchpadPinchEvent>> pinch_queue_;
   std::unique_ptr<QueuedTouchpadPinchEvent> pinch_event_awaiting_ack_;
+  base::Optional<blink::WebMouseWheelEvent> wheel_event_awaiting_ack_;
   base::Optional<bool> first_event_prevented_;
 
   DISALLOW_COPY_AND_ASSIGN(TouchpadPinchEventQueue);

content/browser/renderer_host/input/touchpad_pinch_event_queue_unittest.cc

@@ -103,7 +103,10 @@ class TouchpadPinchEventQueueTest : public testing::TestWithParam<bool> {
 
   void SendWheelEventAck(InputEventAckSource ack_source,
                          InputEventAckState ack_result) {
-    queue_->ProcessMouseWheelAck(ack_source, ack_result, ui::LatencyInfo());
+    const MouseWheelEventWithLatencyInfo mouse_event_with_latency_info(
+        queue_->get_wheel_event_awaiting_ack_for_testing(), ui::LatencyInfo());
+    queue_->ProcessMouseWheelAck(ack_source, ack_result,
+                                 mouse_event_with_latency_info);
   }
 
   base::test::ScopedFeatureList scoped_feature_list_;
@@ -611,4 +614,53 @@ TEST_P(TouchpadPinchEventQueueTest, DoubleTap) {
                     INPUT_EVENT_ACK_STATE_CONSUMED);
 }
 
+// Ensure that ACKs are only processed when they match the event that is
+// currently awaiting an ACK.
+TEST_P(TouchpadPinchEventQueueTest, IgnoreNonMatchingEvents) {
+  ::testing::InSequence sequence;
+  EXPECT_CALL(mock_client_,
+              OnGestureEventForPinchAck(
+                  EventHasType(blink::WebInputEvent::kGesturePinchBegin),
+                  InputEventAckSource::BROWSER, INPUT_EVENT_ACK_STATE_IGNORED));
+  EXPECT_CALL(mock_client_,
+              SendMouseWheelEventForPinchImmediately(
+                  ::testing::AllOf(EventHasCtrlModifier(), EventIsBlocking())));
+  EXPECT_CALL(
+      mock_client_,
+      OnGestureEventForPinchAck(
+          EventHasType(blink::WebInputEvent::kGesturePinchUpdate),
+          InputEventAckSource::MAIN_THREAD, INPUT_EVENT_ACK_STATE_CONSUMED));
+
+  EXPECT_CALL(mock_client_,
+              SendMouseWheelEventForPinchImmediately(::testing::AllOf(
+                  EventHasCtrlModifier(), ::testing::Not(EventIsBlocking()))));
+
+  EXPECT_CALL(mock_client_,
+              OnGestureEventForPinchAck(
+                  EventHasType(blink::WebInputEvent::kGesturePinchEnd),
+                  InputEventAckSource::BROWSER, INPUT_EVENT_ACK_STATE_IGNORED));
+
+  QueuePinchBegin();
+  QueuePinchUpdate(1.23, false);
+  QueuePinchEnd();
+
+  // Create a fake end event to give to ProcessMouseWheelAck to confirm that
+  // it correctly filters this event out and doesn't start processing the ack.
+  blink::WebMouseWheelEvent fake_end_event(
+      blink::WebInputEvent::kMouseWheel, blink::WebInputEvent::kControlKey,
+      blink::WebInputEvent::GetStaticTimeStampForTests());
+  fake_end_event.dispatch_type = blink::WebMouseWheelEvent::kBlocking;
+  fake_end_event.phase = blink::WebMouseWheelEvent::kPhaseEnded;
+  const MouseWheelEventWithLatencyInfo fake_end_event_with_latency_info(
+      fake_end_event, ui::LatencyInfo());
+  queue_->ProcessMouseWheelAck(InputEventAckSource::MAIN_THREAD,
+                               INPUT_EVENT_ACK_STATE_NOT_CONSUMED,
+                               fake_end_event_with_latency_info);
+
+  SendWheelEventAck(InputEventAckSource::MAIN_THREAD,
+                    INPUT_EVENT_ACK_STATE_CONSUMED);
+  SendWheelEventAck(InputEventAckSource::BROWSER,
+                    INPUT_EVENT_ACK_STATE_IGNORED);
+}
+
 }  // namespace content

content/renderer/input/main_thread_event_queue_unittest.cc

@@ -28,10 +28,6 @@ using blink::WebMouseWheelEvent;
 using blink::WebTouchEvent;
 
 namespace blink {
-bool operator==(const WebMouseWheelEvent& lhs, const WebMouseWheelEvent& rhs) {
-  return memcmp(&lhs, &rhs, lhs.size()) == 0;
-}
-
 bool operator==(const WebTouchEvent& lhs, const WebTouchEvent& rhs) {
   return memcmp(&lhs, &rhs, lhs.size()) == 0;
 }

third_party/blink/public/platform/web_mouse_wheel_event.h

@@ -127,6 +127,17 @@ class WebMouseWheelEvent : public WebMouseEvent {
   bool IsCancelable() const { return dispatch_type == kBlocking; }
 #endif
 };
+
+inline bool operator==(const WebMouseWheelEvent& a,
+                       const WebMouseWheelEvent& b) {
+  return memcmp(&a, &b, a.size()) == 0;
+}
+
+inline bool operator!=(const WebMouseWheelEvent& a,
+                       const WebMouseWheelEvent& b) {
+  return !(a == b);
+}
+
 #pragma pack(pop)
 
 }  // namespace blink