Commit e1460a2f authored by Xida Chen's avatar Xida Chen Committed by Commit Bot

Revert "Remove TouchActionFilter::num_of_active_touches_"

This reverts commit bf305c6a.

Reason for revert: cause crash

Original change's description:
> Remove TouchActionFilter::num_of_active_touches_
>
> This variable was added to test whether the touch start / end can be
> acked and released out of order or not. From the reading of the code,
> I don't think is true.
>
> This CL removes this variable, we will monitor crash reports to ensure
> this doesn't cause regression.
>
> To ensure the same behavior, we need a new variable, which is named
> |touch_sequence_in_progress_| in this CL. This is specifically for this
> scenario: we have received touch event ack for touch start, and then
> JS removes / adds touch event listener, which triggers a
> OnHasTouchEventHandlers call. In this case, we should not reset
> touch action if the |touch_sequence_in_pgoress_| is true. We have the
> unit test OnHasTouchEventHandlersReceivedAfterTouchStart to ensure
> this behavior.
>
> Bug: 900195
> Change-Id: I53c3141905b4e2a49ab6e62ea43335c4cee17c15
> Reviewed-on: https://chromium-review.googlesource.com/c/1307250
> Commit-Queue: Xida Chen <xidachen@chromium.org>
> Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#604260}

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

Bug: 901147
Change-Id: I4cd8387cd368eb6ca99010585ea872685953ac89
Reviewed-on: https://chromium-review.googlesource.com/c/1313841Reviewed-by: default avatarDave Tapuska <dtapuska@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604929}
parent 238e6e19
...@@ -331,7 +331,7 @@ void InputRouterImpl::OnTouchEventAck(const TouchEventWithLatencyInfo& event, ...@@ -331,7 +331,7 @@ void InputRouterImpl::OnTouchEventAck(const TouchEventWithLatencyInfo& event,
base::NumberToString(ack_result).c_str()); base::NumberToString(ack_result).c_str());
touch_action_filter_.AppendToGestureSequenceForDebugging( touch_action_filter_.AppendToGestureSequenceForDebugging(
base::NumberToString(event.event.unique_touch_event_id).c_str()); base::NumberToString(event.event.unique_touch_event_id).c_str());
touch_action_filter_.SetTouchSequenceInProgress(true); touch_action_filter_.IncreaseActiveTouches();
// There are some cases the touch action may not have value when receiving // There are some cases the touch action may not have value when receiving
// the ACK for the touch start, such as input ack state is // the ACK for the touch start, such as input ack state is
// NO_CONSUMER_EXISTS, or the renderer has swapped out. In these cases, set // NO_CONSUMER_EXISTS, or the renderer has swapped out. In these cases, set
...@@ -347,7 +347,7 @@ void InputRouterImpl::OnTouchEventAck(const TouchEventWithLatencyInfo& event, ...@@ -347,7 +347,7 @@ void InputRouterImpl::OnTouchEventAck(const TouchEventWithLatencyInfo& event,
touch_action_filter_.AppendToGestureSequenceForDebugging("E"); touch_action_filter_.AppendToGestureSequenceForDebugging("E");
touch_action_filter_.AppendToGestureSequenceForDebugging( touch_action_filter_.AppendToGestureSequenceForDebugging(
base::NumberToString(event.event.unique_touch_event_id).c_str()); base::NumberToString(event.event.unique_touch_event_id).c_str());
touch_action_filter_.SetTouchSequenceInProgress(false); touch_action_filter_.DecreaseActiveTouches();
touch_action_filter_.ReportAndResetTouchAction(); touch_action_filter_.ReportAndResetTouchAction();
UpdateTouchAckTimeoutEnabled(); UpdateTouchAckTimeoutEnabled();
} }
......
...@@ -414,7 +414,7 @@ class InputRouterImplTestBase : public testing::Test { ...@@ -414,7 +414,7 @@ class InputRouterImplTestBase : public testing::Test {
disposition_handler_->GetAndResetAckCount(); disposition_handler_->GetAndResetAckCount();
} }
void TouchSequenceInProgressTest( void ActiveTouchSequenceCountTest(
const base::Optional<cc::TouchAction>& touch_action, const base::Optional<cc::TouchAction>& touch_action,
InputEventAckState state) { InputEventAckState state) {
PressTouchPoint(1, 1); PressTouchPoint(1, 1);
...@@ -423,13 +423,11 @@ class InputRouterImplTestBase : public testing::Test { ...@@ -423,13 +423,11 @@ class InputRouterImplTestBase : public testing::Test {
input_router_->TouchEventHandled( input_router_->TouchEventHandled(
TouchEventWithLatencyInfo(touch_event_), InputEventAckSource::BROWSER, TouchEventWithLatencyInfo(touch_event_), InputEventAckSource::BROWSER,
ui::LatencyInfo(), state, overscroll, touch_action); ui::LatencyInfo(), state, overscroll, touch_action);
EXPECT_TRUE( EXPECT_EQ(input_router_->touch_action_filter_.num_of_active_touches_, 1);
input_router_->touch_action_filter_.touch_sequence_in_progress_);
ReleaseTouchPoint(0); ReleaseTouchPoint(0);
input_router_->OnTouchEventAck(TouchEventWithLatencyInfo(touch_event_), input_router_->OnTouchEventAck(TouchEventWithLatencyInfo(touch_event_),
InputEventAckSource::BROWSER, state); InputEventAckSource::BROWSER, state);
EXPECT_FALSE( EXPECT_EQ(input_router_->touch_action_filter_.num_of_active_touches_, 0);
input_router_->touch_action_filter_.touch_sequence_in_progress_);
} }
void OnTouchEventAckWithAckState(InputEventAckState ack_state) { void OnTouchEventAckWithAckState(InputEventAckState ack_state) {
...@@ -632,29 +630,29 @@ TEST_P(InputRouterImplTest, CoalescesWheelEvents) { ...@@ -632,29 +630,29 @@ TEST_P(InputRouterImplTest, CoalescesWheelEvents) {
// not ACKed from the main thread. // not ACKed from the main thread.
TEST_P(InputRouterImplTest, ActiveTouchSequenceCountWithoutTouchAction) { TEST_P(InputRouterImplTest, ActiveTouchSequenceCountWithoutTouchAction) {
base::Optional<cc::TouchAction> touch_action; base::Optional<cc::TouchAction> touch_action;
TouchSequenceInProgressTest(touch_action, ActiveTouchSequenceCountTest(touch_action,
INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING); INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING);
} }
TEST_P(InputRouterImplTest, TEST_P(InputRouterImplTest,
ActiveTouchSequenceCountWithoutTouchActionNoConsumer) { ActiveTouchSequenceCountWithoutTouchActionNoConsumer) {
base::Optional<cc::TouchAction> touch_action; base::Optional<cc::TouchAction> touch_action;
TouchSequenceInProgressTest(touch_action, ActiveTouchSequenceCountTest(touch_action,
INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS); INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS);
} }
// Test that the active touch sequence count increment when the touch start is // Test that the active touch sequence count increment when the touch start is
// ACKed from the main thread. // ACKed from the main thread.
TEST_P(InputRouterImplTest, ActiveTouchSequenceCountWithTouchAction) { TEST_P(InputRouterImplTest, ActiveTouchSequenceCountWithTouchAction) {
base::Optional<cc::TouchAction> touch_action(cc::kTouchActionPanY); base::Optional<cc::TouchAction> touch_action(cc::kTouchActionPanY);
TouchSequenceInProgressTest(touch_action, ActiveTouchSequenceCountTest(touch_action,
INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING); INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING);
} }
TEST_P(InputRouterImplTest, ActiveTouchSequenceCountWithTouchActionNoConsumer) { TEST_P(InputRouterImplTest, ActiveTouchSequenceCountWithTouchActionNoConsumer) {
base::Optional<cc::TouchAction> touch_action(cc::kTouchActionPanY); base::Optional<cc::TouchAction> touch_action(cc::kTouchActionPanY);
TouchSequenceInProgressTest(touch_action, ActiveTouchSequenceCountTest(touch_action,
INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS); INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS);
} }
TEST_P(InputRouterImplTest, TouchActionAutoWithAckStateConsumed) { TEST_P(InputRouterImplTest, TouchActionAutoWithAckStateConsumed) {
......
...@@ -188,7 +188,10 @@ FilterGestureEventResult TouchActionFilter::FilterGestureEvent( ...@@ -188,7 +188,10 @@ FilterGestureEventResult TouchActionFilter::FilterGestureEvent(
gesture_sequence_in_progress_ = true; gesture_sequence_in_progress_ = true;
// If the gesture is hitting a region that has a non-blocking (such as a // If the gesture is hitting a region that has a non-blocking (such as a
// passive) event listener. // passive) event listener.
if (gesture_event->is_source_touch_event_set_non_blocking) // In theory, the num_of_active_touches_ should be > 0 at this point. But
// crash reports suggest otherwise.
if (gesture_event->is_source_touch_event_set_non_blocking ||
num_of_active_touches_ <= 0)
SetTouchAction(cc::kTouchActionAuto); SetTouchAction(cc::kTouchActionAuto);
active_touch_action_ = allowed_touch_action_; active_touch_action_ = allowed_touch_action_;
if (active_touch_action_.has_value()) if (active_touch_action_.has_value())
...@@ -266,9 +269,12 @@ void TouchActionFilter::OnSetTouchAction(cc::TouchAction touch_action) { ...@@ -266,9 +269,12 @@ void TouchActionFilter::OnSetTouchAction(cc::TouchAction touch_action) {
active_touch_action_ = allowed_touch_action_; active_touch_action_ = allowed_touch_action_;
} }
void TouchActionFilter::SetTouchSequenceInProgress( void TouchActionFilter::IncreaseActiveTouches() {
bool touch_sequence_in_progress) { num_of_active_touches_++;
touch_sequence_in_progress_ = touch_sequence_in_progress; }
void TouchActionFilter::DecreaseActiveTouches() {
num_of_active_touches_--;
} }
void TouchActionFilter::ReportAndResetTouchAction() { void TouchActionFilter::ReportAndResetTouchAction() {
...@@ -277,8 +283,8 @@ void TouchActionFilter::ReportAndResetTouchAction() { ...@@ -277,8 +283,8 @@ void TouchActionFilter::ReportAndResetTouchAction() {
else else
gesture_sequence_.append("RN"); gesture_sequence_.append("RN");
ReportTouchAction(); ReportTouchAction();
DCHECK(!touch_sequence_in_progress_); if (num_of_active_touches_ <= 0)
ResetTouchAction(); ResetTouchAction();
} }
void TouchActionFilter::ReportTouchAction() { void TouchActionFilter::ReportTouchAction() {
...@@ -390,7 +396,7 @@ void TouchActionFilter::OnHasTouchEventHandlers(bool has_handlers) { ...@@ -390,7 +396,7 @@ void TouchActionFilter::OnHasTouchEventHandlers(bool has_handlers) {
// We have set the associated touch action if the touch start already happened // 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 // or there is a gesture in progress. In these cases, we should not reset the
// associated touch action. // associated touch action.
if (!gesture_sequence_in_progress_ && !touch_sequence_in_progress_) { if (!gesture_sequence_in_progress_ && num_of_active_touches_ <= 0) {
ResetTouchAction(); ResetTouchAction();
if (has_touch_event_handler_) if (has_touch_event_handler_)
active_touch_action_.reset(); active_touch_action_.reset();
......
...@@ -60,7 +60,8 @@ class CONTENT_EXPORT TouchActionFilter { ...@@ -60,7 +60,8 @@ class CONTENT_EXPORT TouchActionFilter {
void OnHasTouchEventHandlers(bool has_handlers); void OnHasTouchEventHandlers(bool has_handlers);
void SetTouchSequenceInProgress(bool touch_sequence_in_progress); void IncreaseActiveTouches();
void DecreaseActiveTouches();
void ForceResetTouchActionForTest(); void ForceResetTouchActionForTest();
...@@ -110,8 +111,8 @@ class CONTENT_EXPORT TouchActionFilter { ...@@ -110,8 +111,8 @@ class CONTENT_EXPORT TouchActionFilter {
// before GSE. // before GSE.
bool gesture_sequence_in_progress_ = false; bool gesture_sequence_in_progress_ = false;
// True at touch sequence start and false at touch sequence end. // Increment at receiving ACK for touch start and decrement at touch end.
bool touch_sequence_in_progress_ = false; int num_of_active_touches_ = 0;
// What touch actions are currently permitted. // What touch actions are currently permitted.
base::Optional<cc::TouchAction> allowed_touch_action_; base::Optional<cc::TouchAction> allowed_touch_action_;
......
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