Harden the SimpleScaleGestureDectector against missed events

RESOLVED FIXED in Firefox 16

Status

()

Firefox for Android
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mbrubeck, Assigned: mbrubeck)

Tracking

15 Branch
Firefox 16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
In bug 707571 I came across a case where SimpleScaleGestureDectector's list of active pointers was incorrect, because one of the pointers it was tracking sent an event which another gesture dectector consumed.

In bug 707571 comment 41, kats suggested:
> We should probably harden the SimpleScaleGestureDetector against this case in general,
> by clearing the list of stored touch points before processing an ACTION_DOWN event.

If this works, we can remove the work-around added here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce227a3c263a#l2.74

Assigning to myself so I don't forget to do this, but feel free to steal this if you want to fix it while I'm on vacation.
(Assignee)

Comment 1

6 years ago
By the way, the original bug was fixed upstream in newer versions of Android:
https://github.com/android/platform_frameworks_base/commit/17921eef

If I back out my fix then I can reproduce it on Android 2.3, but not on Android 4.0.
(Assignee)

Comment 2

6 years ago
Created attachment 634068 [details] [diff] [review]
patch
Attachment #634068 - Flags: review?(bugmail.mozilla)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Comment on attachment 634068 [details] [diff] [review]
patch

Review of attachment 634068 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/ui/SimpleScaleGestureDetector.java
@@ +50,5 @@
>      /** Forward touch events to this function. */
>      public void onTouchEvent(MotionEvent event) {
>          switch (event.getAction() & MotionEvent.ACTION_MASK) {
>          case MotionEvent.ACTION_DOWN:
> +            mPointerInfo.clear();

Instead of just clearing the list here, I think we want to run through the code in onTouchEnd pretending that a cancel event came in. In particular we shoudl recycle the pointerInfo objects and send a EventType.END scale gesture to the listener if there are stored pointers.
Attachment #634068 - Flags: review?(bugmail.mozilla) → review-
(Assignee)

Comment 4

6 years ago
Created attachment 634495 [details] [diff] [review]
patch v2

(In reply to Kartikaya Gupta (:kats) from comment #3)
> Instead of just clearing the list here, I think we want to run through the
> code in onTouchEnd pretending that a cancel event came in.

Done.
Attachment #634068 - Attachment is obsolete: true
Attachment #634495 - Flags: review?(bugmail.mozilla)
Comment on attachment 634495 [details] [diff] [review]
patch v2

Review of attachment 634495 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thanks!
Attachment #634495 - Flags: review?(bugmail.mozilla) → review+

Comment 7

6 years ago
https://hg.mozilla.org/mozilla-central/rev/dfa91708c0e1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
You need to log in before you can comment on or make changes to this bug.