1465864 - Re-enable test_group_hittest.html for QR platforms

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement, P3)

Description

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

In bug 1436287 I disabled test_group_hittest.html for webrender because of high frequency failures. Now that async scene building has been turned on in CI we should be able to re-enable this.

Comment 1

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

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

Comment 2

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

That try push looks good, the only orange is a different pre-existing issue.

Comment 3

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

Attachment: Bug 1465864 - Make hit-testing mochitest failure messages nicer.

Instead of printing out base-10 numbers like "expected 67" this will
print out a nicer "expected VISIBLE | DISPATCH_TO_CONTENT | SCROLLBAR"
which makes it easier to see which flags are missing from the hit info.

Review commit: https://reviewboard.mozilla.org/r/248450/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248450/

Comment 4

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

Attachment: Bug 1465864 - Update mochitest code for changes in bug 1443424.

In bug 1443424, the CompositorHitTestInfo for a thumb frame was modified
to only contain the thumb flag if it was layerized. The mochitests were
disabled at the time and so didn't make a fuss.

Review commit: https://reviewboard.mozilla.org/r/248452/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248452/

Comment 5

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

Attachment: Bug 1465864 - Enable test_group_hittest for webrender.

Review commit: https://reviewboard.mozilla.org/r/248454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248454/

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8982481 [details]
Bug 1465864 - Make hit-testing mochitest failure messages nicer.

https://reviewboard.mozilla.org/r/248450/#review254856

Displaying mismatching values in string form is a nice touch :)

::: gfx/layers/apz/test/mochitest/apz_test_utils.js:548
(Diff revision 1)
>  }
>  
> +// Returns a canonical stringification of the hitInfo bitfield.
> +function hitInfoToString(hitInfo) {
> +  var strs = [];
> +  for (var flag in APZHitResultFlags) {

I'm a bit confused about the semantics here. You can iterate over a WebIDL namespace, and the elements of that iteration are the names of the namespace elements in string form? And then you can index into the namespace with these strings to recover the element values?

::: gfx/layers/apz/test/mochitest/apz_test_utils.js:566
(Diff revision 1)
> +
> +// Takes an object returned by hitTest, along with the expected values, and
> +// asserts that they match. Notably, it uses hitInfoToString to provide a
> +// more useful message for the case that the hit info doesn't match
> +function checkHitResult(hitResult, expectedHitInfo, expectedScrollId, desc) {
> +  is(hitInfoToString(hitResult.hitInfo), hitInfoToString(expectedHitInfo), desc + " hit info");

The fact that we're using the strings for the comparison instead of just for output grates a bit, but ok, it's just test code.

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8982482 [details]
Bug 1465864 - Update mochitest code for changes in bug 1443424.

https://reviewboard.mozilla.org/r/248452/#review254862

Always nice to find out a change you've made allows simplifying things elsewhere :)

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8982483 [details]
Bug 1465864 - Enable test_group_hittest for webrender.

https://reviewboard.mozilla.org/r/248454/#review254864

\o/

Comment 9

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

(In reply to Botond Ballo [:botond] from comment #6)
> I'm a bit confused about the semantics here. You can iterate over a WebIDL
> namespace, and the elements of that iteration are the names of the namespace
> elements in string form? And then you can index into the namespace with
> these strings to recover the element values?

Yeah, basically. In JS there is no "namespace" concept, I think it just gets mapped to a regular object with enumerable properties. So it's equivalent to `APZHitResultFlags["INVISIBLE"] = 0` and so on.

> ::: gfx/layers/apz/test/mochitest/apz_test_utils.js:566
> > +function checkHitResult(hitResult, expectedHitInfo, expectedScrollId, desc) {
> > +  is(hitInfoToString(hitResult.hitInfo), hitInfoToString(expectedHitInfo), desc + " hit info");
> 
> The fact that we're using the strings for the comparison instead of just for
> output grates a bit, but ok, it's just test code.

Yeah I was trying to avoid that, but the is() function prints the arguments directly and that's where we want the strings :/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46bd77a82886
Make hit-testing mochitest failure messages nicer. r=botond
https://hg.mozilla.org/integration/autoland/rev/b9580410937c
Update mochitest code for changes in bug 1443424. r=botond
https://hg.mozilla.org/integration/autoland/rev/7305a5747306
Enable test_group_hittest for webrender. r=botond

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/46bd77a82886
https://hg.mozilla.org/mozilla-central/rev/b9580410937c
https://hg.mozilla.org/mozilla-central/rev/7305a5747306