Closed Bug 1467867 Opened 2 years ago Closed 2 years ago
Frame Parent is improperly skipping the event regions override flags
46 bytes, text/x-phabricator-request
|Details | Review|
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.
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+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/192713c16fc1 Ensure we populate the event regions overrides properly. r=botond
Sounds like this can ride the trains if it doesn't like manifest in a user-visible way.
You need to log in before you can comment on or make changes to this bug.