Propagate event region override flags to webrender correctly

RESOLVED FIXED in mozilla58

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

Other Branch
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 unaffected, firefox58 unaffected)

Details

(Whiteboard: [wr-mvp][gfx-noted])

Attachments

(6 attachments)

In gecko we currently set event region override flags on the layer tree in three places: [1] sets it on the root of the layer tree in a given process, [2] sets it on layers corresponding to in-process subdocuments, and [3] sets it on reflayers for out-of-process subdocuments.

However, in the WR code path we only do it for the root [4] and out-of-process subdocuments [5]. So we have a bug where the override isn't set for in-process subdocuments.

While looking at this I realized that the purpose of the override is really to propagate changes to the event regions across the process boundary, so we should only really ever need to set it for the OOP subdocument case and we should be able to get rid of the other two cases. This would, as a side-effect, fix the WR bug because it would eliminate the need to handle the in-process subdocument case in WR and bring the two implementations back in sync.

This change would also make it easier to move hit-testing into WR because we would have fewer overrides being passed around; in fact we would only have overrides per RefLayer which could be viewed as only having overrides per layer tree or pipeline id.

[1] http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/layout/painting/nsDisplayList.cpp#2474-2479
[2] http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/layout/painting/nsDisplayList.cpp#6870
[3] http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/layout/ipc/RenderFrameParent.cpp#378
[4] http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/gfx/layers/wr/WebRenderCommandBuilder.cpp#86
[5] http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/layout/ipc/RenderFrameParent.cpp#410
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [gfx-noted][wr-mvp][triage] → [wr-mvp][gfx-noted]
Now with a couple of new tests that give me a little bit more confidence I didn't break stuff.
Comment on attachment 8926000 [details]
Bug 1415225 - Remove dead code.

https://reviewboard.mozilla.org/r/197234/#review202428
Attachment #8926000 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8926001 [details]
Bug 1415225 - Stop setting the EventRegionsOverride flag on in-process subdocument layers.

https://reviewboard.mozilla.org/r/197236/#review202774
Attachment #8926001 - Flags: review?(botond) → review+
Comment on attachment 8926002 [details]
Bug 1415225 - Stop setting the EventRegionsOverride flag on root layers.

https://reviewboard.mozilla.org/r/197238/#review202778

The change being made to the rebuild-the-displaylist-from-scratch codepath looks good, but I'd like Matt to have a look to make sure we don't need a corresponding change to the retained-displaylist codepath (to e.g. update the flag if it has changed since the last paint).
Attachment #8926002 - Flags: review?(botond) → review+
Attachment #8926002 - Flags: review?(matt.woodrow)
Comment on attachment 8926003 [details]
Bug 1415225 - Move the EventRegionsOverride field to be on RefLayers only.

https://reviewboard.mozilla.org/r/197240/#review202508

Please update the comment above the definition of EventRegionsOverride in LayersTypes.h, which currently says, "Bit flags that go on a ContainerLayer (or RefLayer) ..."

::: commit-message-1657a:3
(Diff revision 2)
> +Bug 1415225 - Move the EventRegionsOverride field to be on RefLayers only. r?botond
> +
> +We now only set EventRegionsOverride flags on ref layers only, so

No need to say "only" twice

::: gfx/layers/apz/src/APZCTreeManager.cpp:654
(Diff revision 2)
>    // Make it so that if the flag is set on the layer tree, it automatically
>    // propagates to all the nodes in the corresponding subtree rooted at that
>    // layer in the hit-test tree. This saves having to walk up the tree every
>    // we want to see if a hit-test node is affected by this flag.
>    EventRegionsOverride result = aLayer.GetEventRegionsOverride();
> +  if (result) {

I'd prefer making this check more explicit:

  if (result != EventRegionsOverride::NoOveride)
  
(I view the fact that NoOverride has the numerical value 0 as an implementation detail. In particular, if it were an |enum class|, it would not implicitly convert to its numerical value and the check as written wouldn't compile.)
Attachment #8926003 - Flags: review?(botond) → review+
Comment on attachment 8926004 [details]
Bug 1415225 - Small early-exit optimization.

https://reviewboard.mozilla.org/r/197242/#review202784
Attachment #8926004 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #25)
> 
> Please update the comment above the definition of EventRegionsOverride in
> LayersTypes.h, which currently says, "Bit flags that go on a ContainerLayer
> (or RefLayer) ..."

Fixed

> 
> No need to say "only" twice

Fixed

> > +  if (result) {
> 
> I'd prefer making this check more explicit:
> 
>   if (result != EventRegionsOverride::NoOveride)

Agreed, fixed as well.
Comment on attachment 8926047 [details]
Bug 1415225 - Add a couple of mochitests for event-regions overrides.

https://reviewboard.mozilla.org/r/197274/#review202988

::: gfx/layers/apz/test/mochitest/helper_override_root.html:21
(Diff revision 1)
> +    dump("Wheel listener running...\n");
> +
> +    // spin for 2 seconds to give APZ time to scroll, if the event region override
> +    // is broken and it decides not to wait for the main thread.
> +    var now = Date.now();
> +    while (Date.now() - now < 2000);

If the parent process is busy for some reason, could APZ not get around to processing the scroll and sending the scroll offset update in 2 seconds?

(If that's the case, it's not a big deal - it would just make the test fail intermittently instead of consistently if the functionality being tested is regressed - but it might be worth a comment.)

::: gfx/layers/apz/test/mochitest/helper_override_root.html:36
(Diff revision 1)
> +
> +function scrollPage() {
> +  var transformEnd = function() {
> +    SpecialPowers.Services.obs.removeObserver(transformEnd, "APZ:TransformEnd", false);
> +    dump("Transform complete; flushing repaints...\n");
> +    flushApzRepaints(checkScroll);

Why is checkScroll, which calls subtestDone, called via two different codepaths?
Comment on attachment 8926047 [details]
Bug 1415225 - Add a couple of mochitests for event-regions overrides.

https://reviewboard.mozilla.org/r/197274/#review202988

> If the parent process is busy for some reason, could APZ not get around to processing the scroll and sending the scroll offset update in 2 seconds?
> 
> (If that's the case, it's not a big deal - it would just make the test fail intermittently instead of consistently if the functionality being tested is regressed - but it might be worth a comment.)

Yeah, this is possible. I'll add a comment.

> Why is checkScroll, which calls subtestDone, called via two different codepaths?

Oh good catch! This is a copy-paste error, we don't need the APZ:TransformEnd observer at all because the page isn't going to scroll and there will be no APZ transform happening. It was in the test I copied from and I guess I missed removing it :)
Comment on attachment 8926047 [details]
Bug 1415225 - Add a couple of mochitests for event-regions overrides.

https://reviewboard.mozilla.org/r/197274/#review202994

Thanks. r+ with those issues addressed.
Attachment #8926047 - Flags: review?(botond) → review+
Comment on attachment 8926002 [details]
Bug 1415225 - Stop setting the EventRegionsOverride flag on root layers.

https://reviewboard.mozilla.org/r/197238/#review203034

::: layout/base/nsLayoutUtils.cpp:3811
(Diff revision 3)
>        }
>  
>        if (!merged) {
>          list.DeleteAll(&builder);
>          builder.EnterPresShell(aFrame);
> +        builder.SetAncestorHasApzAwareEventHandler(

Can we set this earlier? Before the call to AttemptPartialUpdate?

Either that, or also set it within AttemptPartialUpdate.
Attachment #8926002 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #39)
> Can we set this earlier? Before the call to AttemptPartialUpdate?

Yup, I can do that.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4704645d4e81
Remove dead code. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/5f14fc545115
Stop setting the EventRegionsOverride flag on in-process subdocument layers. r=botond
https://hg.mozilla.org/integration/autoland/rev/8ae80d7421d5
Stop setting the EventRegionsOverride flag on root layers. r=botond,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/8cfef302b710
Move the EventRegionsOverride field to be on RefLayers only. r=botond
https://hg.mozilla.org/integration/autoland/rev/9e6710586531
Small early-exit optimization. r=botond
https://hg.mozilla.org/integration/autoland/rev/27403a9dec13
Add a couple of mochitests for event-regions overrides. r=botond
Backed out for frequently failing own gfx/layers/apz/test/mochitest/test_group_overrides.html on Windows:

https://hg.mozilla.org/integration/autoland/rev/95985f660be18466ff7d4aa1c696e73cdebc3ef8

Push with failures (there only in test-verify): https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=27403a9dec13a8d6c37b764782db51ca81ce9436&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log from a normal mochitest run on the next push: https://treeherder.mozilla.org/logviewer.html#?job_id=143237024&repo=autoland

05:23:34     INFO -  1758 INFO TEST-START | gfx/layers/apz/test/mochitest/test_group_overrides.html
05:23:35     INFO -  GECKO(13284) | Flushed APZ repaints, waiting for callback...
05:23:35     INFO -  GECKO(13284) | Finished native wheel, waiting for listener to run...
05:23:35     INFO -  GECKO(13284) | Wheel listener running...
05:23:37     INFO -  GECKO(13284) | Flushed APZ repaints, waiting for callback...
05:23:39     INFO -  GECKO(13284) | Running inside an iframe! stealing functions from window.top...
05:29:00     INFO -  TEST-INFO | started process screenshot
05:29:00     INFO -  TEST-INFO | screenshot: exit 0
05:29:00     INFO -  Buffered messages logged at 05:23:34
05:29:00     INFO -  1759 INFO must wait for load
05:29:00     INFO -  Buffered messages logged at 05:23:37
05:29:00     INFO -  1760 INFO TEST-PASS | gfx/layers/apz/test/mochitest/test_group_overrides.html | helper_override_root.html | check that the window didn't scroll
05:29:00     INFO -  Buffered messages logged at 05:23:39
05:29:00     INFO -  1761 INFO must wait for load
05:29:00     INFO -  1762 INFO must wait for focus
05:29:00     INFO -  Buffered messages finished
05:29:00    ERROR -  1763 INFO TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/test_group_overrides.html | Test timed out.
05:29:00     INFO -      reportError@SimpleTest/TestRunner.js:121:7
05:29:00     INFO -      TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7
05:29:00     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
05:29:00     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
05:29:00     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
05:29:00     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
05:29:00     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
05:29:00     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
05:29:00     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
05:29:00     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
05:29:00     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
05:29:00     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
05:29:00     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
05:29:00     INFO -      TestRunner.runTests@SimpleTest/TestRunner.js:380:5
05:29:00     INFO -      RunSet.runtests@SimpleTest/setup.js:194:3
05:29:00     INFO -      RunSet.runall@SimpleTest/setup.js:173:5
05:29:00     INFO -      hookupTests@SimpleTest/setup.js:266:5
05:29:00     INFO -  parseTestManifest@http://mochi.test:8888/manifestLibrary.js:36:5
05:29:00     INFO -  getTestManifest/req.onload@http://mochi.test:8888/manifestLibrary.js:49:11
05:29:00     INFO -  EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3
05:29:00     INFO -      hookup@SimpleTest/setup.js:246:5
05:29:00     INFO -  EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&manifestFile=tests.json&dumpOutputDirectory=c%3A%5Cusers%5Cgenericworker%5Cappdata%5Clocal%5Ctemp&cleanupCrashes=true:11:1
05:29:01     INFO -  Not taking screenshot here: see the one that was previously logged
Flags: needinfo?(bugmail)
Looks like sometimes the iframe doesn't get focused and so the test hangs. I could repro locally on Windows - not really sure why it happens but adding an onload attribute on the <iframe> that explicitly focuses the iframe element fixes it.
Flags: needinfo?(bugmail)
Also for the record it looks like the focus problem also happened once on OS X, https://treeherder.mozilla.org/logviewer.html#?job_id=143273741&repo=autoland&lineNumber=6628 - most frequent on Windows though, and I didn't see it at all on Linux.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3cac8cc22a73
Remove dead code. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/ebd75f69dd52
Stop setting the EventRegionsOverride flag on in-process subdocument layers. r=botond
https://hg.mozilla.org/integration/autoland/rev/4cbed02f2878
Stop setting the EventRegionsOverride flag on root layers. r=botond,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/f92965e9eb3c
Move the EventRegionsOverride field to be on RefLayers only. r=botond
https://hg.mozilla.org/integration/autoland/rev/646849f41754
Small early-exit optimization. r=botond
https://hg.mozilla.org/integration/autoland/rev/9d4871427e54
Add a couple of mochitests for event-regions overrides. r=botond
You need to log in before you can comment on or make changes to this bug.