1139155 - Add APZ tests that exercise the full input stack

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

The DOMWindowUtils.sendTouchEvent is implemented in a way that allows injecting touch events directly into the widget code. On B2G this is the best way we have to simulate real user input in a mochitest/reftest scenario, and we should write tests that take advantage of this.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Hook up native touch event synthesization for child processes

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Add a reliable mechanism to know when APZ is done

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - First sanity test

Comment 4

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddc93861714d

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8572191 [details] [diff] [review]
Part 1 - Hook up native touch event synthesization for child processes

Moved this patch to bug 1146349 and added other input types as well.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Add a reliable mechanism to know when APZ is done

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Basic touch-scroll sanity test

Dusted off these patches and got them working locally again. Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0037a199e82 which will almost certainly fail (that's what happened last time) at which point I'll have to spend some quality time with a releng loaner machine.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Whoops, wrong try push. Right one is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a1feb602398

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Amazingly that try push was green. I want to refactor the code a little bit and add some more similar tests before putting it up for review.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Add a reliable mechanism to know when APZ is done

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Add basic panning tests

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8624731 [details] [diff] [review]
Part 2 - Add basic panning tests

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

::: gfx/layers/apz/test/apz_test_native_event_utils.js
@@ +142,5 @@
> +
> +function synthesizeNativeDrag(aElement, aX, aY, aDeltaX, aDeltaY, aObserver = null, aTouchId = 0) {
> +  // We wait for the touchstart to be processed so that gecko has time to
> +  // layerize whatever it needs to layerize before we flood it with events to
> +  // process.

Whoops, ignore this comment. I'll take it out, it got left behind from a previous version.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba2adbb3df53

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8624731 [details] [diff] [review]
Part 2 - Add basic panning tests

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

Nice!

::: gfx/layers/apz/test/apz_test_native_event_utils.js
@@ +126,5 @@
>  }
> +
> +// Synthesizes a native touch event and dispatches it. aX and aY in CSS pixels
> +// relative to the top-left of |aElement|'s bounding rect.
> +function synthesizeNativeTouch(aElement, aX, aY, aType, aObserver = null, aTouchId = 0) {

Based on usage it seems like having aObserver at the end would make more sense.

@@ +132,5 @@
> +
> +  var scale = targetWindow.devicePixelRatio;
> +  var rect = aElement.getBoundingClientRect();
> +  var x = targetWindow.mozInnerScreenX + ((rect.left + aX) * scale);
> +  var y = targetWindow.mozInnerScreenY + ((rect.top + aY) * scale);

I'm OK with putting this calculation inside synthesizeNativeTouch, but for consistency we should put it inside synthesizeNativeWheel and synthesizeNativeMouseMove, too.

@@ +134,5 @@
> +  var rect = aElement.getBoundingClientRect();
> +  var x = targetWindow.mozInnerScreenX + ((rect.left + aX) * scale);
> +  var y = targetWindow.mozInnerScreenY + ((rect.top + aY) * scale);
> +
> +  var utils = SpecialPowers.getDOMWindowUtils(targetWindow);

What's the difference between 'SpecialPowers.getDOMWindowUtils' and the '_getDOMWindowUtils' used in the other synthesizeNative*() functions in this file?

::: gfx/layers/apz/test/helper_iframe_pan.html
@@ +32,5 @@
> +
> +  </script>
> +</head>
> +<body>
> + <iframe id="outer" style="height: 250px; border: solid 1px black" src="data:text/html,<body style='height:5000px'>"></iframe>

Don't need to close the <body> tag?

::: gfx/layers/apz/test/test_basic_pan.html
@@ +43,5 @@
> +        ["apz.touch_start_tolerance", "0.0"],
> +
> +        // The B2G emulator is hella slow, and needs more than 300ms to run the
> +        // main-thread code that deals with layerizing subframes and running
> +        // touch listeners. In my local runs this needs to be at least 1000.

Do we have touch listeners in these tests?

Comment 15

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #14)
> > +// Synthesizes a native touch event and dispatches it. aX and aY in CSS pixels
> > +// relative to the top-left of |aElement|'s bounding rect.
> > +function synthesizeNativeTouch(aElement, aX, aY, aType, aObserver = null, aTouchId = 0) {
> 
> Based on usage it seems like having aObserver at the end would make more
> sense.

Not sure I understood the "based on usage" part. I assume you're referring to the order of aObserver and aTouchId - but in the tests I have here both aObserver and aTouchId are always unspecified at the call site, so it shouldn't matter what order they are. In general I figured aObserver would be non-null more frequently than aTouchId since the latter would only be specified for multitouch tests. That's why I put it last.

> > +  var x = targetWindow.mozInnerScreenX + ((rect.left + aX) * scale);
> > +  var y = targetWindow.mozInnerScreenY + ((rect.top + aY) * scale);
> 
> I'm OK with putting this calculation inside synthesizeNativeTouch, but for
> consistency we should put it inside synthesizeNativeWheel and
> synthesizeNativeMouseMove, too.

Agreed, I can do that in a followup after you land your test to avoid rebasing issues.

> > +
> > +  var utils = SpecialPowers.getDOMWindowUtils(targetWindow);
> 
> What's the difference between 'SpecialPowers.getDOMWindowUtils' and the
> '_getDOMWindowUtils' used in the other synthesizeNative*() functions in this
> file?

I think they're pretty much the same (see [1]) but to have access to _getDOMWindowUtils you need to include EventUtils.js, which I didn't include in the helper files. I can do that and switch to using _getDOMWindowUtils if you prefer. Or I can switch the existing _getDOMWindowUtils to use SpecialPowers.

[1] http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js?rev=f06affb13067#924

> ::: gfx/layers/apz/test/helper_iframe_pan.html
> > +<body>
> > + <iframe id="outer" style="height: 250px; border: solid 1px black" src="data:text/html,<body style='height:5000px'>"></iframe>
> 
> Don't need to close the <body> tag?

Not really, it will get closed automatically by the parser at the end of the document.

> > +        // The B2G emulator is hella slow, and needs more than 300ms to run the
> > +        // main-thread code that deals with layerizing subframes and running
> > +        // touch listeners. In my local runs this needs to be at least 1000.
> 
> Do we have touch listeners in these tests?

Not explicitly, but we do rely on the layerizing of subframes in these tests. When I was looking at the layer dumps though I noticed that the force-dispatch-to-content flag was set so it might be that the mochitest harness registers a listener of some kind that is forcing d-t-c.

Comment 16

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> (In reply to Botond Ballo [:botond] from comment #14)
> > > +// Synthesizes a native touch event and dispatches it. aX and aY in CSS pixels
> > > +// relative to the top-left of |aElement|'s bounding rect.
> > > +function synthesizeNativeTouch(aElement, aX, aY, aType, aObserver = null, aTouchId = 0) {
> > 
> > Based on usage it seems like having aObserver at the end would make more
> > sense.
> 
> Not sure I understood the "based on usage" part. I assume you're referring
> to the order of aObserver and aTouchId - but in the tests I have here both
> aObserver and aTouchId are always unspecified at the call site, so it
> shouldn't matter what order they are. In general I figured aObserver would
> be non-null more frequently than aTouchId since the latter would only be
> specified for multitouch tests. That's why I put it last.

I was referring to the call sites in synthesizeNativeDrag() which pass a touch id but not an observer, but if you expect future usage to pass an observer more frequently, then this is fine. In any case, it's a very small thing :)

> > > +  var x = targetWindow.mozInnerScreenX + ((rect.left + aX) * scale);
> > > +  var y = targetWindow.mozInnerScreenY + ((rect.top + aY) * scale);
> > 
> > I'm OK with putting this calculation inside synthesizeNativeTouch, but for
> > consistency we should put it inside synthesizeNativeWheel and
> > synthesizeNativeMouseMove, too.
> 
> Agreed, I can do that in a followup after you land your test to avoid
> rebasing issues.

Sounds good, thanks.

> > > +
> > > +  var utils = SpecialPowers.getDOMWindowUtils(targetWindow);
> > 
> > What's the difference between 'SpecialPowers.getDOMWindowUtils' and the
> > '_getDOMWindowUtils' used in the other synthesizeNative*() functions in this
> > file?
> 
> I think they're pretty much the same (see [1]) but to have access to
> _getDOMWindowUtils you need to include EventUtils.js, which I didn't include
> in the helper files. I can do that and switch to using _getDOMWindowUtils if
> you prefer. Or I can switch the existing _getDOMWindowUtils to use
> SpecialPowers.

I'd prefer that we be consistent one way or the other. We can do that in the follow-up, too.

> > > +        // The B2G emulator is hella slow, and needs more than 300ms to run the
> > > +        // main-thread code that deals with layerizing subframes and running
> > > +        // touch listeners. In my local runs this needs to be at least 1000.
> > 
> > Do we have touch listeners in these tests?
> 
> Not explicitly, but we do rely on the layerizing of subframes in these
> tests.

Ah, good point!

Comment 17

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #16)
> I was referring to the call sites in synthesizeNativeDrag() which pass a
> touch id but not an observer, but if you expect future usage to pass an
> observer more frequently, then this is fine. In any case, it's a very small
> thing :)

Ah gotcha. Yeah I think going forward we'll need the observer more frequently. We might even need it in synthesizeNativeDrag to make the test work on Windows. I think I'll leave it as is for now.

> > Agreed, I can do that in a followup after you land your test to avoid
> > rebasing issues.
> 
> Sounds good, thanks.

Filed bug 1176402 for this.

> > I think they're pretty much the same (see [1]) but to have access to
> > _getDOMWindowUtils you need to include EventUtils.js, which I didn't include
> > in the helper files. I can do that and switch to using _getDOMWindowUtils if
> > you prefer. Or I can switch the existing _getDOMWindowUtils to use
> > SpecialPowers.
> 
> I'd prefer that we be consistent one way or the other. We can do that in the
> follow-up, too.

This is also covered by bug 1176402.

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8624731 [details] [diff] [review]
Part 2 - Add basic panning tests

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

::: gfx/layers/apz/test/test_basic_pan.html
@@ +47,5 @@
> +        // touch listeners. In my local runs this needs to be at least 1000.
> +        ["apz.content_response_timeout", "5000"]
> +      ]
> +    }, testDone);
> +}

My fancy new JS IDE is telling me you're missing a semicolon here :)

Comment 19

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #18)
> My fancy new JS IDE is telling me you're missing a semicolon here :)

Heh. I can add it, but JS has automatic semicolon insertion so really your IDE is wrong :)

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23b7cd26fa9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/1339cd0ef472

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/23b7cd26fa9a
https://hg.mozilla.org/mozilla-central/rev/1339cd0ef472