Closed Bug 1419834 Opened 7 years ago Closed 7 years ago

Add ability to exercise APZ hit-testing code from mochitests

Categories

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

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files, 1 obsolete file)

Bug 1419834 - Fix missing newline in logging code.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1419834 - Add hit-testing results for MozMouseHittest events to APZTestData.
59 bytes, text/x-review-board-request
botond
: review+
mrbkap
: review+
Details
Bug 1419834 - Add a mochitest for compositor hit-testing.
59 bytes, text/x-review-board-request
botond
: review+
Details 
It would be really useful to specifically exercise the hit-testing code in APZ from mochitests. I envision a DOMWindowUtils API that takes a point and returns some useful info that comes out of the APZ hit-test that we can check for correctness. This would be useful not just for the existing hit-testing code but also to verify that the WR hit-testing code is producing correct values. And most importantly, it will allow adding tests to go with patches, so that we don't regress behaviour later (which is easy to do today).
Priority: -- → P3
Whiteboard: [gfx-noted]
Comment on attachment 8932151 [details]
Bug 1419834 - Fix missing newline in logging code.

https://reviewboard.mozilla.org/r/203190/#review208586
Attachment #8932151 - Flags: review?(botond) → review+
One of my hopes when I introduced APZTestData in bug 961289, was that it would allow us to avoid having to do all the plumbing the second patch in this bug does, for every type of information we want to surface in a test.

Have we considered leveraging that by writing the test along these lines:

  - In the C++ hit testing code, if APZ test logging is enabled, 
    record the result of the hit test in the APZ test data

  - In the test:

      - trigger a hit test by e.g. synthesizing a native mouse
        event

      - look at the APZ test data to check the result of the hit
        testing

?
Comment on attachment 8932153 [details]
Bug 1419834 - Add sync-messages annotations for new sync IPC.

https://reviewboard.mozilla.org/r/203194/#review208596
Attachment #8932153 - Flags: review?(dvander) → review+
(In reply to Botond Ballo [:botond] from comment #8)
> One of my hopes when I introduced APZTestData in bug 961289, was that it
> would allow us to avoid having to do all the plumbing the second patch in
> this bug does, for every type of information we want to surface in a test.
> 
> Have we considered leveraging that by writing the test along these lines:

I hadn't considered this, but I'm trying it now. It's looking like there's still a bunch of plumbing required since the existing APZTestData has data structures set up per-paint and I would want this to be independent of paints. But still overall it might be less plumbing. I'll post a patch when I have it working.
I got it basically working: https://github.com/staktrace/gecko-dev/commit/95f76536ca6b6943ceb8312f74b12f93423e4fbb

The only thing that I couldn't figure out is how to expose the constants via the APZTestData.webidl file. I can only put const attributes inside an "interface" but putting the interface into the webidl file doesn't compile as it looks for a mozilla/dom/APZHitResultFlags.h file which doesn't exist. Any thoughts on that?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> The only thing that I couldn't figure out is how to expose the constants via
> the APZTestData.webidl file. I can only put const attributes inside an
> "interface" but putting the interface into the webidl file doesn't compile
> as it looks for a mozilla/dom/APZHitResultFlags.h file which doesn't exist.
> Any thoughts on that?

I think an interface needs to be backed by a C++ object that you define (rather than having its definition be generated). You can control the type of the C++ object, and the header file it's defined in, by adding appropriate entries to dom/bindings/Bindings.conf [1].

I'm basing this on this documentation [2]; other issues you might run into are likely to be covered there as well.

[1] https://searchfox.org/mozilla-central/source/dom/bindings/Bindings.conf
[2] https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f0a778faee8f0bae2b84752e8bf2d707e4d8d73

Fails on android because we don't have a good mechanism for sending mouse events through to APZ. I'll just skip-if android the test for now; it should be possible to plumb the hit-test event through there as well but is probably better to do in a separate bug.
Attachment #8932153 - Attachment is obsolete: true
@mrbkap - I need a DOM peer to review the webidl change. I picked you since you were the last one to review a change APZTestData.webidl, but feel free to pass it to somebody else.
Comment on attachment 8932152 [details]
Bug 1419834 - Add hit-testing results for MozMouseHittest events to APZTestData.

https://reviewboard.mozilla.org/r/203192/#review209052

::: dom/webidl/APZTestData.webidl:51
(Diff revision 2)
> +};
> +
> +dictionary APZHitResult {
> +  float screenX;
> +  float screenY;
> +  unsigned short hitResult; // combination of the HITTEST_* flags

Oh I need to fix this comment. It should say "combination of the APZHitResultFlags.* flags"
Comment on attachment 8932152 [details]
Bug 1419834 - Add hit-testing results for MozMouseHittest events to APZTestData.

https://reviewboard.mozilla.org/r/203192/#review209486
Attachment #8932152 - Flags: review?(botond) → review+
Comment on attachment 8932154 [details]
Bug 1419834 - Add a mochitest for compositor hit-testing.

https://reviewboard.mozilla.org/r/203196/#review209498

Nice test :)

::: gfx/layers/apz/test/mochitest/helper_hittest_basic.html:33
(Diff revision 2)
> +
> +function hitTest(point) {
> +  dump("Hit-testing point (" + point.x + ", " + point.y + ")\n");
> +  utils.sendMouseEvent("MozMouseHittest", point.x, point.y, 0, 0, 0, true, 0, 0, true, true);
> +  var data = utils.getCompositorAPZTestData();
> +  var result = data.hitResults[data.hitResults.length - 1];

Maybe assert it's not empty, to get a better error message if it is?

::: gfx/layers/apz/test/mochitest/helper_hittest_basic.html:89
(Diff revision 2)
> +    var verticalScrollbarPoint = {
> +        x: scroller.getBoundingClientRect().right - (verticalScrollbarWidth / 2),
> +        y: scroller.getBoundingClientRect().y + scrollbarArrowButtonHeight + 5,
> +    };
> +    var {hitInfo, scrollId} = hitTest(verticalScrollbarPoint);
> +    is(hitInfo, APZHitResultFlags.VISIBLE | APZHitResultFlags.DISPATCH_TO_CONTENT | APZHitResultFlags.SCROLLBAR | APZHitResultFlags.SCROLLBAR_VERTICAL | inactiveScrollframeThumbFlag,

wrap line
Attachment #8932154 - Flags: review?(botond) → review+
Comment on attachment 8932152 [details]
Bug 1419834 - Add hit-testing results for MozMouseHittest events to APZTestData.

https://reviewboard.mozilla.org/r/203192/#review209586
Attachment #8932152 - Flags: review?(mrbkap) → review+
Addressed review comments. Final try push to verify the (minor) test changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b33ec5ae1016f69b182c3d3e900df36f02a0656c - is green.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a5545cca107
Fix missing newline in logging code. r=botond
https://hg.mozilla.org/integration/autoland/rev/03d2ac8e6eef
Add hit-testing results for MozMouseHittest events to APZTestData. r=botond,mrbkap
https://hg.mozilla.org/integration/autoland/rev/8f2cee5cde44
Add a mochitest for compositor hit-testing. r=botond
Depends on: 1436831
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: