Closed Bug 1037409 Opened 6 years ago Closed 6 years ago

Remove touch-action gtests that don't make sense

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(1 file)

There are a few gtests that use "default gestures" (i.e. don't use the APZC's gesture detector, but send in gesture events directly) and that also set allowed touch behavior for touch-action. I don't think these tests make sense because this will never happen in the real world.

If we sending pinch events directly into the APZC, that means we cannot reliably also be sending touch behaviour. For example on Mac doing a pinch on the trackpad can generate a pinch with the correct focus point, but you cannot reliably map the user's fingers to two touch points for which you can determine touch behaviour. I think that if the APZC gets a pinch gesture event from the widget code, it must just handle it unconditionally. If the platform wants to enforce touch-action behavior in this case, then it should send in the individual touch points rather than the pre-detected PinchGestureEvent.
Attached patch PatchSplinter Review
Attachment #8454428 - Flags: review?(nicklebedev37)
Comment on attachment 8454428 [details] [diff] [review]
Patch

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

>> For example on Mac doing a pinch on the trackpad can generate a pinch with the correct focus point, but you cannot reliably map the user's fingers to two touch points for which you can determine touch behaviour. I think that if the APZC gets a pinch gesture event from the widget code, it must just handle it unconditionally.

If it is a real world use-case then we might need to make sure that Apzc's code will handle it correctly. I believe it will because if Apzc is provided with the Pinch Gestures [1] it won't start WaitingContentResponse timer and will check in the [2] an empty array which will return true. But it's preliminary thoughts.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp#903
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp#2528

About tests - I agree, it's ok to remove them.
Attachment #8454428 - Flags: review?(nicklebedev37) → review+
(In reply to Nick Lebedev [:nl] from comment #2)
> If it is a real world use-case then we might need to make sure that Apzc's
> code will handle it correctly. I believe it will because if Apzc is provided
> with the Pinch Gestures [1] it won't start WaitingContentResponse timer and
> will check in the [2] an empty array which will return true. But it's
> preliminary thoughts.

I agree that we need to make sure that APZC handles this correctly. But right now the code is a mess and I'm in the process of cleaning it up. I'll be adding more tests that make sense as part of the cleanup. See bug 1009733 and the bugs it depends on for more details.
https://hg.mozilla.org/mozilla-central/rev/3a497072902b
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.