1372912 - Make layout/reftests/async-scrolling/bg-fixed-child-mask.html pass on linux64-qr with APZ enabled

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P3)

Description

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

With WR+APZ enabled, the layout/reftests/async-scrolling/bg-fixed-child-mask.html reftest fails. This bug is for the patches that make it pass, without regressing any "earlier" tests.

Comment 1

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

Try pushes with [1] and without [2] APZ enabled. These patches build on top of the patches in bug 1372603.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e132d25bf457b12b99da61fe55a9feb5411c864
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=580e12b2b102bb71b46ec021344725d360b6545a

Comment 2

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

Argh. Let's try that again.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6baca4fc50642ef8eaf54dea8efeab8a6aac028
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d278e395a5fdb2efbedfc14f65e166ab67a09614

Comment 3

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

Those look good. (The APZ-enabled push is expected to have failures, as long as bg-fixed-child-mask.html isn't among them).

Comment 4

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

Attachment: Bug 1372912 - Extract a PushLayerClip helper method.

No functional changes. We'll want to reuse this code in a future patch.

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

Comment 5

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

Attachment: Bug 1372912 - Push clips for all the scroll metadata scrollclips.

Each layer has a stack of scroll metadata objects, which represent the
set of scrollframes that got flattened into the layer. Effectively the
metadata objects provide the scroll information for the chain of
scrollframes containing that layer's content. Each of these scrollframes
may have a clip, and so we need to push those clips to WR. We need to
take care to insert these clips at the right point in the stack, so that
they are interleaved correctly with the PushScrollLayer calls that we
use to inform WR of the scrolling clips for the scrollframes. This patch
implements this behaviour.

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

Comment 6

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

Attachment: Bug 1372912 - Make the clip id more strongly typed.

In order to fully support some background-attachment:fixed scenarios, we
need to be able to call PushClipAndScrollInfo with two non-scrolling
clips. Currently the PushClipAndScrollInfo API requires that the scroll
clip be a FrameMetrics::ViewID which represents a scrolling clip.
Therefore this patch takes the first step in loosening the API so we can
pass either a scrolling or non-scrolling clip. The next patch will
complete this transition.

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

Comment 7

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

Attachment: Bug 1372912 - Place the layer local clip in the right spot for fixed-pos layers.

This patch modifies PushClipAndScrollInfo so that it can take either a
scrolling or a non-scrolling clip as the first argument. The scrolling
clip is represented by the FrameMetrics::ViewID variant, and the
non-scrolling clip is represented by the WrClipId variant.

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

Comment 8

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

Attachment: Bug 1372912 - Place the layer local clip in the right spot for fixed-pos layers.

This patch makes the bg-fixed-child-mask.html reftest pass by ensuring
the clips for the test case are all in the correct spots. See the
comments in the patch for a more detailed explanation.

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

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8877698 [details]
Bug 1372912 - Extract a PushLayerClip helper method.

https://reviewboard.mozilla.org/r/149134/#review153598

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8877699 [details]
Bug 1372912 - Push clips for all the scroll metadata scrollclips.

https://reviewboard.mozilla.org/r/149136/#review153600

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8877700 [details]
Bug 1372912 - Make the clip id more strongly typed.

https://reviewboard.mozilla.org/r/149138/#review153602

Comment 12

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

After some IRC discussion with Martin, he explained that the "branch" thing I'm doing is not necessary and that soon passing a non-scrolling clip to PushClipAndScrollInfo won't work. So I removed those bits and the test still passes. I feel like we might run into scenarios we can't handle, but maybe we never create the type of layer tree I'm imagining. I'm not sure. Anyway, we can deal with those scenarios if/when we run into them.

For now I removed part 4 (I could have removed part 3 as well but figured it wouldn't hurt to keep it) and simplified part 5.

Try pushes with and without APZ enabled:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9d69a1622096bd1958cc4d2731319a5b0188fac
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f67f70827f91098113bd67a544b153952f203828

Comment 13

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

Those try pushes look good. Patches incoming.

Comment 14

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

Comment on attachment 8877698 [details]
Bug 1372912 - Extract a PushLayerClip helper method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149134/diff/1-2/

Comment 15

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

Comment on attachment 8877699 [details]
Bug 1372912 - Push clips for all the scroll metadata scrollclips.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149136/diff/1-2/

Comment 16

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

Comment on attachment 8877700 [details]
Bug 1372912 - Make the clip id more strongly typed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149138/diff/1-2/

Comment 17

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

Comment on attachment 8877701 [details]
Bug 1372912 - Place the layer local clip in the right spot for fixed-pos layers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149140/diff/1-2/

Comment 18

[Martin Robinson (email:[e-mail])]

Comment on attachment 8877701 [details]
Bug 1372912 - Place the layer local clip in the right spot for fixed-pos layers.

https://reviewboard.mozilla.org/r/149140/#review153960

This seems good to me. Feel free to wait for jrmuizel's review if you would like someone more familiar with Gecko code to review this as well.

Comment 19

[Martin Robinson (email:[e-mail])]

Comment on attachment 8877700 [details]
Bug 1372912 - Make the clip id more strongly typed.

https://reviewboard.mozilla.org/r/149138/#review153962

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b883786cf01
Extract a PushLayerClip helper method. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/6f148794682a
Push clips for all the scroll metadata scrollclips. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/8176859f8b70
Make the clip id more strongly typed. r=jrmuizel,mrobinson
https://hg.mozilla.org/integration/autoland/rev/18c0e82c745f
Place the layer local clip in the right spot for fixed-pos layers. r=mrobinson

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for bustage at ScrollingLayersHelper.h:19: class 'LayerClip' was previously declared as a struct:

https://hg.mozilla.org/integration/autoland/rev/53067b8193dc849e693f4fff553b7149dea93509
https://hg.mozilla.org/integration/autoland/rev/656f10819385a03675d066e5d0abaf351a33677f
https://hg.mozilla.org/integration/autoland/rev/00dbcf762ef556226b350cfe82d3656db300de35
https://hg.mozilla.org/integration/autoland/rev/9ee1932fe123b0ba818a95d4c610001b097e8a43

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=18c0e82c745fe3b54b3ae07f83f013d3ef8cca35&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=107449744&repo=autoland

13:52:04     INFO -  Unified_cpp_gfx_layers10.cpp
13:52:04     INFO -  c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\obj-firefox\dist\include\mozilla/layers/ScrollingLayersHelper.h(19): error C2220: warning treated as error - no 'object' file generated
13:52:04     INFO -  c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\obj-firefox\dist\include\mozilla/layers/ScrollingLayersHelper.h(19): warning C4099: 'mozilla::layers::LayerClip': type name first seen using 'struct' now seen using 'class'
13:52:04     INFO -  c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\gfx\layers\FrameMetrics.h(711): note: see declaration of 'mozilla::layers::LayerClip'
13:52:04     INFO -  c:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/config/rules.mk:1005: recipe for target 'Unified_cpp_gfx_layers10.obj' failed
13:52:04     INFO -  mozmake.EXE[5]: *** [Unified_cpp_gfx_layers10.obj] Error 2

Comment 22

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

Sorry, silly error. My compiler (and try pushes) didn't catch it.

Comment 23

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

Comment on attachment 8877698 [details]
Bug 1372912 - Extract a PushLayerClip helper method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149134/diff/2-3/

Comment 24

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

Comment on attachment 8877699 [details]
Bug 1372912 - Push clips for all the scroll metadata scrollclips.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149136/diff/2-3/

Comment 25

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

Comment on attachment 8877700 [details]
Bug 1372912 - Make the clip id more strongly typed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149138/diff/2-3/

Comment 26

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

Comment on attachment 8877701 [details]
Bug 1372912 - Place the layer local clip in the right spot for fixed-pos layers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149140/diff/2-3/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/037a153bb989
Extract a PushLayerClip helper method. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/b65804983bc8
Push clips for all the scroll metadata scrollclips. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/39215a3e0a45
Make the clip id more strongly typed. r=jrmuizel,mrobinson
https://hg.mozilla.org/integration/autoland/rev/622f421e5815
Place the layer local clip in the right spot for fixed-pos layers. r=mrobinson

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/037a153bb989
https://hg.mozilla.org/mozilla-central/rev/b65804983bc8
https://hg.mozilla.org/mozilla-central/rev/39215a3e0a45
https://hg.mozilla.org/mozilla-central/rev/622f421e5815