Closed Bug 1467867 Opened Last year Closed Last year

RenderFrameParent is improperly skipping the event regions override flags

Categories

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

60 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 1 obsolete file)

https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/layout/ipc/RenderFrameParent.cpp#353

The IsBuildingLayerEventRegions() check here doesn't return true nowadays, because we create nsDisplayCompositorHitTestInfo items instead of nsDisplayLayerEventRegions items. This is true for both webrender and non-webrender (layers). In the layers codepath, those HitTestInfo items are still collapsed onto the layers, and APZ uses the resulting EventRegions objects for hit testing. This means we still need the EventRegionsOverride flags set on the layers. But the flags getting set are conditioned on IsBuildingLayerEventRegions, so they're not getting set.

The bug is caused by a sloppy use of the IsBuildingLayerEventRegions condition (which it looks like I added back in the day), which hasn't been updated as the code evolved. If there are resulting user-visible regressions (which is unlikely) they would have manifested in Fx 60 onwards, when we turned on the "simple event region items" pref in bug 1434243. It probably doesn't actually manifest anywhere user-visible; the override flags were more important back in the B2G days when the UI process would do things that triggered these flags. But still, we should fix it.
Assignee: nobody → bugmail
The event regions override flags are needed whenever APZ is doing
hit-testing off the layer tree. This can happen even if
IsBuildingLayerEventRegions() returns false, because we can instead be
building nsDisplayCompositorHitTestInfo items from which we populate the
layer tree's EventRegions objects. So the guard condition here is wrong,
and we can just remove it to ensure the flags are always put on the
layer tree. If APZ isn't enabled on this layer tree then they won't be
used, and there might be a slight perf hit, but it should be negligible.
Good lord, Phabricator's sub-line diffing is atrocious.

Please excuse the churn while I upload this patch to MozReview, so that I can take a screenshot of MozReview's diff, so that I can file a bug about how atrocious Phabricator's sub-line diffing is with illustrative screenshots.
Oh look, all you're doing is removing a condition. (That wasn't clear at all from the Phabricator diff.)
Comment on attachment 8984565 [details]
Ensure we populate the event regions overrides properly.

Botond Ballo [:botond] [standards meeting June 4-9] has approved the revision.

https://phabricator.services.mozilla.com/D1599
Attachment #8984565 - Flags: review+
Attachment #8984576 - Attachment is obsolete: true
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/192713c16fc1
Ensure we populate the event regions overrides properly. r=botond
https://hg.mozilla.org/mozilla-central/rev/192713c16fc1
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Sounds like this can ride the trains if it doesn't like manifest in a user-visible way.
Blocks: 1444845
You need to log in before you can comment on or make changes to this bug.