Bug 101219 – Layout Test compositing/repaint/invalidations-on-composited-layers.html is failing/flaky

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Stephen White]

The following layout test is flaky on all platforms

compositing/repaint/invalidations-on-composited-layers.html

Probable cause:

Nothing obvious; WebKit (r133458) and Chrome (r165894) changes around the start of flakiness seem innocuous.

[Stephen White]

CC'ing vollick, since this test was introduced at http://trac.webkit.org/changeset/133332, and perhaps has been just intermittently flaky since then.

Created attachment 172796 [details]
Patch

The test is flaky because I'd forgotten to force a commit. This is normally done
via testRunner.display(), but that's problematic for a couple of reasons. First,
we're trying to stop using display altogether in favour of text-based repaint
testing. It's also a problem because testRunner.display() has unfortunate side
effects in addition to forcing a composite. For example, on mac it starts
repaint tracking and resets our stored repaint rects, making our test useless.

What we really need is a new method, testRunner.forceComposite(). This should
eventually replace testRunner.display() in all composited tests (since that was
always the real intent of using display there).

This CL adds the forceComposite method and uses it in the flaky test.

[EFL EWS Bot]

Comment on attachment 172796 [details]
Patch

Attachment 172796 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14757408

[Simon Fraser (smfr)]

Comment on attachment 172796 [details]
Patch

Is Chromium the only platform that requires special behavior here?
Can you just force the compositing update before dumping repaint rects (and layer tree)? I'd prefer this be something "automatic", rather than something that test authors have to think about, particularly if a test that passes on one platform is going to fail on another.

[Build Bot]

Comment on attachment 172796 [details]
Patch

Attachment 172796 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14763253

[Simon Fraser (smfr)]

Comment on attachment 172796 [details]
Patch

r- for now.

Created attachment 173638 [details]
Patch

It turns out that it's not a composite that I'm after here. testRunner.display()
causes a layout on chromium, and it's that layout that affects the lists of
invalidation rects. This layout can be forced simply by forcing a style recalc
in the usual way -- it just needs to happen before we start repaint tracking.

So although it would be nice to add a testRunner.forceComposite, it's not
required to fix the flake we're seeing here.

[James Robinson]

Comment on attachment 173638 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173638&action=review

> LayoutTests/compositing/repaint/invalidations-on-composited-layers.html:48
> +          var dummy = child.offsetTop;

just "document.body.offsetTop;" as a statement is sufficient, you don't have to declare a var

Created attachment 173678 [details]
Patch

(In reply to comment #8)
> (From update of attachment 173638 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173638&action=review
>
> > LayoutTests/compositing/repaint/invalidations-on-composited-layers.html:48
> > +          var dummy = child.offsetTop;
>
> just "document.body.offsetTop;" as a statement is sufficient, you don't have to declare a var

Thanks, that's much nicer.

[James Robinson]

Comment on attachment 173678 [details]
Patch

R=me although I wonder if this should be implicit as part of starting/stopping tracking.

(In reply to comment #10)
> (From update of attachment 173678 [details])
> R=me although I wonder if this should be implicit as part of starting/stopping tracking.

I was also thinking about automatically forcing a style recalc at start/stop and just before dumping the layer tree as text, but I was worried that there may be cases where we intentionally want to avoid forcing a style update. Maybe that's not a big concern though? I'll close this for now, but I'd be happy to open a bug if we decide to make the style recalcs implicit.

[WebKit Review Bot]

Comment on attachment 173678 [details]
Patch

Clearing flags on attachment: 173678

Committed r134287: <http://trac.webkit.org/changeset/134287>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.