Bug 103786 – Use background color for GraphicsLayers when applicable

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Noam Rosenthal]

In some cases, a GraphicsLayer can benefit from directly compositing its background color rather than painting it together with the rest of the box decorations.
This can later expand to tiled background images, gradients, border images, border radius, borders/outlines, etc. but for now solid color is the lowest hanging fruit.

[Noam Rosenthal]

Created attachment 177049 [details]
Patch for EWS / initial review of direction

[Simon Fraser (smfr)]

Comment on attachment 177049 [details]
Patch for EWS / initial review of direction

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

> Source/WebCore/ChangeLog:15
> +        No new tests. Covered by many existing tests, though some would require rebaseline.
> +        (current patch is for EWS, and for direction review).
> +

That's not true at all on non Qt platforms. You need a whole slew of tests here.

> Source/WebCore/rendering/RenderLayerBacking.cpp:491
> +    if (m_hasDirectlyCompositedBackground)
> +        updateDirectlyCompositedBackground();

You called updateDirectlyCompositedBackground() above. Why is it being called twice?

> Source/WebCore/rendering/RenderLayerBacking.cpp:526
> +    if (shouldUpdateBackgroundColor)
> +        updateBackgroundColor();

This will do the wrong thing for multiple backgrounds with alpha colors, or with different background-clip/background-origin.

> Source/WebCore/rendering/RenderLayerBacking.cpp:773
> +    IntRect contentsBounds = m_hasDirectlyCompositedBackground ? borderBox() : contentsBox();

You need to take background-origin and background-clip style into account.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1130
> +
> +

Blank lines.

[Noam Rosenthal]

No need to put an r- on a patch that didn't have r? :)

(In reply to comment #2)
> (From update of attachment 177049 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177049&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        No new tests. Covered by many existing tests, though some would require rebaseline.
> > +        (current patch is for EWS, and for direction review).
> > +
> 
> That's not true at all on non Qt platforms. You need a whole slew of tests here.
Sorry, wrong wording. Of course new tests are needed (for Qt as well), I meant to say here that many tests already cover this code path. Hence the patch is not for review yet.
I would like to know where to look for which tests are needed for this.

> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:491
> > +    if (m_hasDirectlyCompositedBackground)
> > +        updateDirectlyCompositedBackground();
> 
> You called updateDirectlyCompositedBackground() above. Why is it being called twice?
Leftover..

> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:526
> > +    if (shouldUpdateBackgroundColor)
> > +        updateBackgroundColor();
> 
> This will do the wrong thing for multiple backgrounds with alpha colors, or with different background-clip/background-origin.
OK, for now I can count those out.

> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:773
> > +    IntRect contentsBounds = m_hasDirectlyCompositedBackground ? borderBox() : contentsBox();
> 
> You need to take background-origin and background-clip style into account.
Right.

[Noam Rosenthal]

Created attachment 177193 [details]
Patch

[Noam Rosenthal]

(In reply to comment #4)
> Created an attachment (id=177193) [details]
> Patch

I'm sure there's some stuff I'm missing here, but I think/hope it's at a point where it's worth a look.

[Build Bot]

Comment on attachment 177193 [details]
Patch

Attachment 177193 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15086843

New failing tests:
css3/filters/filtered-compositing-descendant.html

[WebKit Review Bot]

Comment on attachment 177193 [details]
Patch

Attachment 177193 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15121167

New failing tests:
compositing/backface-visibility/backface-visibility-hierarchical-transform.html
compositing/background-color/background-color-alpha.html
compositing/background-color/background-color-composite.html
animations/3d/matrix-transform-type-animation.html
animations/3d/state-at-end-event-transform.html
compositing/backface-visibility/backface-visibility-non3d.html
compositing/backing/no-backing-for-clip-overlap.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
compositing/animation/state-at-end-event-transform-layer.html
compositing/backface-visibility/backface-visibility-3d.html
compositing/clip-change.html
compositing/background-color/background-color-text-change.html
compositing/background-color/background-color-simple.html
compositing/sibling-positioning.html
compositing/fixed-position-changed-in-composited-layer.html
compositing/background-color/background-color-content-clip.html
compositing/absolute-position-changed-in-composited-layer.html
compositing/flat-with-transformed-child.html
compositing/background-color/background-color-padding-clip.html
compositing/background-color/background-color-container.html
compositing/preserve-3d-toggle.html
compositing/fixed-position-scroll-offset-history-restore.html
compositing/background-color/background-color-text-clip.html
compositing/text-on-scaled-layer.html
compositing/overflow-trumps-transform-style.html
compositing/backing/no-backing-for-clip.html
compositing/backface-visibility/backface-visibility-simple.html
compositing/backing/no-backing-for-perspective.html
animations/animation-offscreen-to-onscreen.html

[Darin Adler]

Comment on attachment 177193 [details]
Patch

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

Patch looks good.

review- mainly because of the questionable design choice to put the call to updateContentsRect inside the updateDrawsContent function. If we want to combine these operations, then the function needs a new name. Or there could be some other solution to that problem.

> Source/WebCore/ChangeLog:40
> +        (WebCore):

Please remove bogus lines like this from change log.

> Source/WebCore/rendering/RenderBox.h:143
>      LayoutRect borderBoxRect() const { return LayoutRect(LayoutPoint(), size()); }
> +    LayoutRect paddingBoxRect() const { return LayoutRect(borderLeft(), borderTop(), contentWidth() + paddingLeft() + paddingRight(), contentHeight() + paddingTop() + paddingBottom()); }
>      IntRect pixelSnappedBorderBoxRect() const { return IntRect(IntPoint(), m_frameRect.pixelSnappedSize()); }

I don’t understand why these function names use the phrase “box rect” instead of just the word “box”.

> Source/WebCore/rendering/RenderLayerBacking.cpp:-761
> -    m_graphicsLayer->setContentsRect(contentsBox());

Per-function comments in the change log would have helped me understand why this change is OK. It’s surprising that there is no call to updateContentsRect here, but I am sure there is some clear simple explanation for that.

> Source/WebCore/rendering/RenderLayerBacking.cpp:805
> +    updateContentsRect();

Again, per-function comments could make this more clear. It doesn’t seem right to just add updateContentsRect here, since the function name makes it sound like this just updates drawsContent. And in fact, before this change that’s exactly what this function did. I think it’s a problem to just add this code here given the function’s name.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1143
> +    if (backgroundColor == Color::transparent)
> +        m_graphicsLayer->clearBackgroundColor();

Why is this needed? Why wasn’t it needed before? Again, per-function comments are a good place to explain such things.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1156
> +    // FIXME: deal with backgroundComposite() correctly.
> +    if (renderer()->style()->backgroundComposite() != CompositeSourceOver)
> +        return true;

This FIXME is not clear. What is incorrect here? Is the issue simply that we don’t optimize non-source-over cases and we’re like to cover more? Or is there something that’s actually incorrect? The comment would be better if it said what was wrong rather than just exhorting us to do things right.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1438
> +IntRect RenderLayerBacking::backgroundBox() const

Why does this return an IntRect? Is “pixel snapped” the correct rounding for this box?

> Source/WebCore/rendering/RenderLayerBacking.cpp:1458
> +    default:
> +        ASSERT_NOT_REACHED();
> +        break;

By including a default case, we lose the warning we’d get if we didn’t cover one of the enumeration values. Often the best style for such things is to make a helper function so we can have the assertion outside the switch statement and use return statements inside the switch statement. That way we get both the assertion at runtime when the value is not a valid enum value, and the compile failure at compile time if we create a new enumeration value and forget to handle it.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1463
> +    IntSize contentOffset = contentOffsetInCompostingLayer();
> +    backgroundBox.move(contentOffset);

I don’t think the local variable for contentOffset here is helpful.

[Noam Rosenthal]

Comment on attachment 177193 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.h:143
>>      IntRect pixelSnappedBorderBoxRect() const { return IntRect(IntPoint(), m_frameRect.pixelSnappedSize()); }
> 
> I don’t understand why these function names use the phrase “box rect” instead of just the word “box”.

Should I address this in this bug? I've been trying to adjust the naming convention to what exists, maybe renaming those should be done in the scope of a separate bug.

>> Source/WebCore/rendering/RenderLayerBacking.cpp:-761
>> -    m_graphicsLayer->setContentsRect(contentsBox());
> 
> Per-function comments in the change log would have helped me understand why this change is OK. It’s surprising that there is no call to updateContentsRect here, but I am sure there is some clear simple explanation for that.

This is OK because we call updateDrawsContent at the end of this function (which would be renamed to updateContentsConfiguration)

>> Source/WebCore/rendering/RenderLayerBacking.cpp:805
>> +    updateContentsRect();
> 
> Again, per-function comments could make this more clear. It doesn’t seem right to just add updateContentsRect here, since the function name makes it sound like this just updates drawsContent. And in fact, before this change that’s exactly what this function did. I think it’s a problem to just add this code here given the function’s name.

Sure thing. 
I will create a new function called updateContentConfiguration that takes care of drawsContent, contentsRect and backgroundColor.

>> Source/WebCore/rendering/RenderLayerBacking.cpp:1143
>> +        m_graphicsLayer->clearBackgroundColor();
> 
> Why is this needed? Why wasn’t it needed before? Again, per-function comments are a good place to explain such things.

Will do.
It wasn't needed before because setContentsToBackgroundColor was never set, except for in the case of full-screen.
The contract between RenderLayerBacking and GraphicsLayer around background-colors was somewhat unexplored before this patch.

>> Source/WebCore/rendering/RenderLayerBacking.cpp:1156
>> +        return true;
> 
> This FIXME is not clear. What is incorrect here? Is the issue simply that we don’t optimize non-source-over cases and we’re like to cover more? Or is there something that’s actually incorrect? The comment would be better if it said what was wrong rather than just exhorting us to do things right.

The issue is that we could cover more but choose to leave that to a future patch, since background-composite is not really used when there is no background image. I will update the comment to be clearer.

[Noam Rosenthal]

Created attachment 177704 [details]
Patch

[WebKit Review Bot]

Comment on attachment 177704 [details]
Patch

Attachment 177704 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15152551

New failing tests:
platform/chromium/virtual/softwarecompositing/overflow/resize-painting.html
compositing/overflow/content-gains-scrollbars.html
compositing/overflow/overflow-scrollbar-layers.html
platform/chromium/virtual/softwarecompositing/overflow/overflow-scrollbar-layers.html
compositing/overflow/resize-painting.html
platform/chromium/virtual/softwarecompositing/overflow/content-gains-scrollbars.html

[Noam Rosenthal]

Created attachment 177867 [details]
Patch

[Noam Rosenthal]

Created attachment 177883 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 177883 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerBacking.cpp:798
> +    if (isSimpleContainerCompositingLayer() && renderer()->hasBackground())

isSimpleContainerCompositingLayer() does quite a bit of work. I wonder if we should cache this or pass it in.

> Source/WebCore/rendering/RenderLayerBacking.cpp:801
> +        m_graphicsLayer->setContentsRect(backgroundBox());
> +    else
> +        m_graphicsLayer->setContentsRect(contentsBox());

I prefer things like this to be written as:

FloatRect contentsRect;
if (...)
    contentsRect = 
else
   contentsRect =

m_graphicsLayer->setContentsRect(contentsRect);

[Noam Rosenthal]

Created attachment 178273 [details]
Patch

[WebKit Review Bot]

Comment on attachment 178273 [details]
Patch

Rejecting attachment 178273 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
commit-queue/Source/WebKit/chromium/third_party/v8-i18n --revision 159 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
50>At revision 159.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/15188467

[James Robinson]

A repository hook failed: Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/Source/WebCore/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/lib/git-core/git-svn line 570

[James Robinson]

Comment on attachment 178273 [details]
Patch

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

> Source/WebCore/ChangeLog:21
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

^^

[Noam Rosenthal]

(In reply to comment #18)
> (From update of attachment 178273 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178273&action=review
> 
> > Source/WebCore/ChangeLog:21
> > +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
> 
> ^^

OOPS :)

[Noam Rosenthal]

Created attachment 178302 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 178302 [details]
Patch for landing

Clearing flags on attachment: 178302

Committed r137006: <http://trac.webkit.org/changeset/137006>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

I believe this change completely broke painting of plugins on Mac, so I'm going to roll it out (I bisected down to this one and http://trac.webkit.org/changeset/137007, and this one seems a bit more likely).

[WebKit Review Bot]

Re-opened since this is blocked by bug 104446

[Noam Rosenthal]

(In reply to comment #23)
> I believe this change completely broke painting of plugins on Mac, so I'm going to roll it out (I bisected down to this one and http://trac.webkit.org/changeset/137007, and this one seems a bit more likely).

Is there a test for this?(In reply to comment #23)
> I believe this change completely broke painting of plugins on Mac, so I'm going to roll it out (I bisected down to this one and http://trac.webkit.org/changeset/137007, and this one seems a bit more likely).

Is there a test I could run to reproduce?

[Tim Horton]

(In reply to comment #25)
> (In reply to comment #23)
> > I believe this change completely broke painting of plugins on Mac, so I'm going to roll it out (I bisected down to this one and http://trac.webkit.org/changeset/137007, and this one seems a bit more likely).
> 
> Is there a test for this?(In reply to comment #23)
> > I believe this change completely broke painting of plugins on Mac, so I'm going to roll it out (I bisected down to this one and http://trac.webkit.org/changeset/137007, and this one seems a bit more likely).
> 
> Is there a test I could run to reproduce?

A test, I'm not sure. There are plugin tests, but I don't know much about them/if they work right now/if they're skipped/whatever! As a manual test, though, just try to load a youtube video.

[Noam Rosenthal]

> A test, I'm not sure. There are plugin tests, but I don't know much about them/if they work right now/if they're skipped/whatever! As a manual test, though, just try to load a youtube video.
OK, I will revise and resubmit. Thank you!

[Noam Rosenthal]

Created attachment 178374 [details]
Patch

[Noam Rosenthal]

(In reply to comment #28)
> Created an attachment (id=178374) [details]
> Patch

updateBackgroundColor was at the wrong place... when I move it to updateGraphicsLayerConfiguration everything works including plugins.

If Simon is ok with this, I will re-land.

[Simon Fraser (smfr)]

Comment on attachment 178374 [details]
Patch

It's a shame that no test caught the plug-in failure. The test plug-in does support CA mode, but I'm not sure if dumping the layer tree would have caught the failure.

[WebKit Review Bot]

Comment on attachment 178374 [details]
Patch

Clearing flags on attachment: 178374

Committed r137051: <http://trac.webkit.org/changeset/137051>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Seems like a few compositing tests need rebaselining after this patch:
http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r137062%20%286839%29/results.html

Diffs look like:
--- /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/layout-test-results/compositing/rtl/rtl-iframe-absolute-expected.txt
+++ /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/layout-test-results/compositing/rtl/rtl-iframe-absolute-actual.txt
@@ -25,7 +25,7 @@
                                 (GraphicsLayer
                                   (position 50.00 50.00)
                                   (bounds 100.00 100.00)
-                                  (drawsContent 1)
+                                  (backgroundColor #008000)
                                 )
                               )
                             )

[Chris Dumez]

(In reply to comment #33)
> Seems like a few compositing tests need rebaselining after this patch:
> http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r137062%20%286839%29/results.html
> 
> Diffs look like:
> --- /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/layout-test-results/compositing/rtl/rtl-iframe-absolute-expected.txt
> +++ /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/layout-test-results/compositing/rtl/rtl-iframe-absolute-actual.txt
> @@ -25,7 +25,7 @@
>                                  (GraphicsLayer
>                                    (position 50.00 50.00)
>                                    (bounds 100.00 100.00)
> -                                  (drawsContent 1)
> +                                  (backgroundColor #008000)
>                                  )
>                                )
>                              )

I rebaselined those tests in http://trac.webkit.org/changeset/137065

[Chris Dumez]

2 of the tests introduced in this patch are failing on WebKit2 EFL:
http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r137066%20(6841)/compositing/background-color/background-color-padding-change-diffs.html
http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r137066%20(6841)/compositing/background-color/background-color-text-change-diffs.html

[Chris Dumez]

(In reply to comment #35)
> 2 of the tests introduced in this patch are failing on WebKit2 EFL:
> http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r137066%20(6841)/compositing/background-color/background-color-padding-change-diffs.html
> http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r137066%20(6841)/compositing/background-color/background-color-text-change-diffs.html

Filed https://bugs.webkit.org/show_bug.cgi?id=104476 to track the issue.

[Beth Dakin]

This change regressed behavior on Mac: https://bugs.webkit.org/show_bug.cgi?id=104942

[Simon Fraser (smfr)]

Followup fixing is happening in bug 104842.

[Alexey Proskuryakov]

This caused bug 104983, not sure if that's already covered by the other follow-up bugs.