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.
Created attachment 177049 [details] Patch for EWS / initial review of direction
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.
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.
Created attachment 177193 [details] Patch
(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.
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
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
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.
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.
Created attachment 177704 [details] Patch
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
Created attachment 177867 [details] Patch
Created attachment 177883 [details] Patch
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);
Created attachment 178273 [details] Patch
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
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
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!). ^^
(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 :)
Created attachment 178302 [details] Patch for landing
Comment on attachment 178302 [details] Patch for landing Clearing flags on attachment: 178302 Committed r137006: <http://trac.webkit.org/changeset/137006>
All reviewed patches have been landed. Closing bug.
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).
Re-opened since this is blocked by bug 104446
(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?
(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.
> 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!
Created attachment 178374 [details] Patch
(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.
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.
Comment on attachment 178374 [details] Patch Clearing flags on attachment: 178374 Committed r137051: <http://trac.webkit.org/changeset/137051>
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) ) ) )
(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
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
(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.
This change regressed behavior on Mac: https://bugs.webkit.org/show_bug.cgi?id=104942
Followup fixing is happening in bug 104842.
This caused bug 104983, not sure if that's already covered by the other follow-up bugs.