New issue
Advanced search Search tips

Issue 900195 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 31
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Remove TouchActionFilter::num_of_active_touches_

Project Member Reported by xidac...@chromium.org, Oct 30

Issue description

This var was added because we suspect that the touch start / end could be acked and released out of order. This is probably not true, we should remove this and observer crashes.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bf305c6a1aca0fafdc643b6639496d345ea50fd6

commit bf305c6a1aca0fafdc643b6639496d345ea50fd6
Author: Xida Chen <xidachen@chromium.org>
Date: Wed Oct 31 15:22:03 2018

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}
[modify] https://crrev.com/bf305c6a1aca0fafdc643b6639496d345ea50fd6/content/browser/renderer_host/input/input_router_impl.cc
[modify] https://crrev.com/bf305c6a1aca0fafdc643b6639496d345ea50fd6/content/browser/renderer_host/input/input_router_impl_unittest.cc
[modify] https://crrev.com/bf305c6a1aca0fafdc643b6639496d345ea50fd6/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/bf305c6a1aca0fafdc643b6639496d345ea50fd6/content/browser/renderer_host/input/touch_action_filter.h
[modify] https://crrev.com/bf305c6a1aca0fafdc643b6639496d345ea50fd6/content/browser/renderer_host/input/touch_action_filter_unittest.cc

Status: Fixed (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e1460a2f329179da1431fa2b6731ce04bb0d71dc

commit e1460a2f329179da1431fa2b6731ce04bb0d71dc
Author: Xida Chen <xidachen@chromium.org>
Date: Fri Nov 02 15:30:20 2018

Revert "Remove TouchActionFilter::num_of_active_touches_"

This reverts commit bf305c6a1aca0fafdc643b6639496d345ea50fd6.

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/1313841
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604929}
[modify] https://crrev.com/e1460a2f329179da1431fa2b6731ce04bb0d71dc/content/browser/renderer_host/input/input_router_impl.cc
[modify] https://crrev.com/e1460a2f329179da1431fa2b6731ce04bb0d71dc/content/browser/renderer_host/input/input_router_impl_unittest.cc
[modify] https://crrev.com/e1460a2f329179da1431fa2b6731ce04bb0d71dc/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/e1460a2f329179da1431fa2b6731ce04bb0d71dc/content/browser/renderer_host/input/touch_action_filter.h
[modify] https://crrev.com/e1460a2f329179da1431fa2b6731ce04bb0d71dc/content/browser/renderer_host/input/touch_action_filter_unittest.cc

Sign in to add a comment