Commit 284cfbd5 authored by Matt Falkenhagen's avatar Matt Falkenhagen Committed by Commit Bot

Revert "Count number of active touches in TouchActionFilter"

This reverts commit 0ee23351.

Reason for revert: Top crash in canary and blocks dev release
per bug 880021.

Original change's description:
> Count number of active touches in TouchActionFilter
> 
> Right now we have a active_touch_in_progress_ to indicate whether there
> is an active touch or not. It is true when we receive ACK for touch
> start and false at touch end.
> 
> From the current crash report, we are seeing multiple consecutive ack
> for touch end, without any ack for touch start in between.
> 
> This CL changes the active_touch_in_progress_ to num_of_active_touches_.
> It is increased by one when we receive ack for touch start and decrease
> by one at touch end. At touch end, we only reset touch action when this
> number is zero.
> 
> In theory, touch start and associated touch end should arrive in pair.
> This CL does a DumpWithoutCrashing if this number is ever larger than
> one.
> 
> Bug: 851644
> Change-Id: I584f1aad56bf5bc5c08e20e3ae4ca36d5f9e3586
> Reviewed-on: https://chromium-review.googlesource.com/1196696
> Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
> Commit-Queue: Xida Chen <xidachen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#587813}

TBR=dtapuska@chromium.org,xidachen@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 851644, 880021
Change-Id: I265b573cef100ff6210683e7f5ec01dfcb28ea9f
Reviewed-on: https://chromium-review.googlesource.com/1205923Reviewed-by: default avatarMatt Falkenhagen <falken@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588791}
parent 51b234e2
......@@ -340,16 +340,16 @@ void InputRouterImpl::OnTouchEventAck(const TouchEventWithLatencyInfo& event,
if (WebTouchEventTraits::IsTouchSequenceStart(event.event) &&
ack_result == INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS) {
touch_action_filter_.AppendToGestureSequenceForDebugging("T");
touch_action_filter_.IncreaseActiveTouches();
// Touch action must be auto when there is no consumer
touch_action_filter_.OnSetTouchAction(cc::kTouchActionAuto);
touch_action_filter_.SetActiveTouchInProgress(true);
UpdateTouchAckTimeoutEnabled();
}
disposition_handler_->OnTouchEventAck(event, ack_source, ack_result);
if (WebTouchEventTraits::IsTouchSequenceEnd(event.event)) {
touch_action_filter_.DecreaseActiveTouches();
touch_action_filter_.ReportAndResetTouchAction();
touch_action_filter_.SetActiveTouchInProgress(false);
UpdateTouchAckTimeoutEnabled();
}
}
......@@ -530,11 +530,8 @@ void InputRouterImpl::TouchEventHandled(
// The SetTouchAction IPC occurs on a different channel so always
// send it in the input event ack to ensure it is available at the
// time the ACK is handled.
if (touch_action.has_value()) {
if (WebTouchEventTraits::IsTouchSequenceStart(touch_event.event))
touch_action_filter_.IncreaseActiveTouches();
if (touch_action.has_value())
OnSetTouchAction(touch_action.value());
}
// |touch_event_queue_| will forward to OnTouchEventAck when appropriate.
touch_event_queue_.ProcessTouchAck(source, state, latency,
......@@ -599,6 +596,7 @@ void InputRouterImpl::OnHasTouchEventHandlers(bool has_handlers) {
void InputRouterImpl::ForceSetTouchActionAuto() {
touch_action_filter_.AppendToGestureSequenceForDebugging("F");
touch_action_filter_.OnSetTouchAction(cc::kTouchActionAuto);
touch_action_filter_.SetActiveTouchInProgress(true);
}
void InputRouterImpl::OnHasTouchEventHandlersForTest(bool has_handlers) {
......@@ -618,6 +616,7 @@ void InputRouterImpl::OnSetTouchAction(cc::TouchAction touch_action) {
touch_action_filter_.AppendToGestureSequenceForDebugging(
std::to_string(touch_action).c_str());
touch_action_filter_.OnSetTouchAction(touch_action);
touch_action_filter_.SetActiveTouchInProgress(true);
// kTouchActionNone should disable the touch ack timeout.
UpdateTouchAckTimeoutEnabled();
......
......@@ -7,7 +7,6 @@
#include <math.h>
#include "base/debug/crash_logging.h"
#include "base/debug/dump_without_crashing.h"
#include "base/logging.h"
#include "base/metrics/histogram_macros.h"
#include "third_party/blink/public/platform/web_gesture_event.h"
......@@ -241,19 +240,9 @@ void TouchActionFilter::OnSetTouchAction(cc::TouchAction touch_action) {
scrolling_touch_action_ = allowed_touch_action_;
}
void TouchActionFilter::IncreaseActiveTouches() {
// The touch start and associated touch end should be acked in order. If not,
// dump.
if (num_of_active_touches_ > 0)
base::debug::DumpWithoutCrashing();
num_of_active_touches_++;
}
void TouchActionFilter::DecreaseActiveTouches() {
// Something is seriously wrong if this is true.
if (num_of_active_touches_ == 0)
base::debug::DumpWithoutCrashing();
num_of_active_touches_--;
void TouchActionFilter::SetActiveTouchInProgress(
bool active_touch_in_progress) {
active_touch_in_progress_ = active_touch_in_progress;
}
void TouchActionFilter::ReportAndResetTouchAction() {
......@@ -262,8 +251,7 @@ void TouchActionFilter::ReportAndResetTouchAction() {
else
gesture_sequence_.append("R0");
ReportTouchAction();
if (num_of_active_touches_ <= 0)
ResetTouchAction();
ResetTouchAction();
}
void TouchActionFilter::ReportTouchAction() {
......@@ -372,7 +360,7 @@ void TouchActionFilter::OnHasTouchEventHandlers(bool has_handlers) {
// We have set the associated touch action if the touch start already happened
// or there is a gesture in progress. In these cases, we should not reset the
// associated touch action.
if (!gesture_sequence_in_progress_ && num_of_active_touches_ <= 0) {
if (!gesture_sequence_in_progress_ && !active_touch_in_progress_) {
ResetTouchAction();
if (has_touch_event_handler_)
scrolling_touch_action_.reset();
......
......@@ -67,8 +67,7 @@ class CONTENT_EXPORT TouchActionFilter {
void OnHasTouchEventHandlers(bool has_handlers);
void IncreaseActiveTouches();
void DecreaseActiveTouches();
void SetActiveTouchInProgress(bool active_touch_in_progress);
// Debugging only.
void AppendToGestureSequenceForDebugging(const char* str);
......@@ -108,8 +107,8 @@ class CONTENT_EXPORT TouchActionFilter {
// before GSE.
bool gesture_sequence_in_progress_ = false;
// Increment at receiving ACK for touch start and decrement at touch end.
int num_of_active_touches_ = 0;
// True at touch start and false at touch end.
bool active_touch_in_progress_ = false;
// What touch actions are currently permitted.
base::Optional<cc::TouchAction> allowed_touch_action_;
......
......@@ -1079,7 +1079,7 @@ TEST_F(TouchActionFilterTest, OnHasTouchEventHandlersReceivedAfterTouchStart) {
// Receive a touch start ack, set the touch action.
filter_.OnSetTouchAction(cc::kTouchActionPanY);
filter_.IncreaseActiveTouches();
filter_.SetActiveTouchInProgress(true);
filter_.OnHasTouchEventHandlers(false);
EXPECT_EQ(ScrollingTouchAction().value(), cc::kTouchActionPanY);
EXPECT_EQ(filter_.allowed_touch_action().value(), cc::kTouchActionPanY);
......@@ -1088,33 +1088,6 @@ TEST_F(TouchActionFilterTest, OnHasTouchEventHandlersReceivedAfterTouchStart) {
EXPECT_EQ(filter_.allowed_touch_action().value(), cc::kTouchActionPanY);
}
TEST_F(TouchActionFilterTest, ResetTouchActionWithActiveTouch) {
filter_.OnHasTouchEventHandlers(true);
EXPECT_FALSE(ScrollingTouchAction().has_value());
EXPECT_FALSE(filter_.allowed_touch_action().has_value());
// Receive a touch start ack, set the touch action.
filter_.OnSetTouchAction(cc::kTouchActionPanY);
filter_.IncreaseActiveTouches();
// Somehow we get the ACK for the second touch start before the ACK for the
// first touch end.
filter_.OnSetTouchAction(cc::kTouchActionPan);
filter_.IncreaseActiveTouches();
// The first touch end comes, we report and reset touch action. The touch
// actions should still have value.
filter_.DecreaseActiveTouches();
filter_.ReportAndResetTouchAction();
EXPECT_EQ(ScrollingTouchAction().value(), cc::kTouchActionPanY);
EXPECT_EQ(filter_.allowed_touch_action().value(), cc::kTouchActionPanY);
// The ack for the second touch end comes, the touch actions will be reset.
filter_.DecreaseActiveTouches();
filter_.ReportAndResetTouchAction();
EXPECT_FALSE(filter_.allowed_touch_action().has_value());
}
// If the renderer is busy, the gesture event might have come before the
// OnHasTouchEventHanlders IPC is received. In this case, we should allow all
// the gestures.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment