Bug 78728 – Box shadow drawing takes an unnecessarily slow code path in some single-shadow, opaque-background cases

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

Box shadow drawing takes an unnecessarily slow code path in some single-shadow, opaque-background cases

<rdar://problem/10870238>

Created attachment 127213 [details]
Let the background cast the shadow directly in some cases where this is possible

[Dave Hyatt]

Comment on attachment 127213 [details]
Let the background cast the shadow directly in some cases where this is possible

It's slightly slower, but you could simplify the patch considerably if you just moved:

if (!boxShadowShouldBeAppliedToBackground(determineBackgroundBleedAvoidance(paintInfo.context)))
    paintBoxShadow(paintInfo, rect, style(), Normal);

inside RenderBoxModelObject::paintBoxShadow.

Then you wouldn't have to patch a bunch of call sites, and you wouldn't even have to touch InlineFlowBox.

I'll leave it up to you, though, since your way is ever-so-slightly faster at the cost of complicating every call site.

I can't do this because InlineFlowBox doesn't use determineBackgroundBleedAvoidance(). Thanks for the review!

Fixed in <http://trac.webkit.org/r107836>.

[Tony Chang]

On the chromium bots, this changed 3 pixel results in unexpected ways.

In fast/box-shadow/spread.html, the black shapes got smaller.
http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/chromium-mac-snowleopard/fast/box-shadow/spread-expected.png
http://build.chromium.org/f/chromium/layout_test_results/Webkit_Mac10_6/results/layout-test-results/fast/box-shadow/spread-actual.png
http://build.chromium.org/f/chromium/layout_test_results/Webkit_Mac10_6/results/layout-test-results/fast/box-shadow/spread-diff.png

In fast/writing-mode/box-shadow-horizontal-bt.html and fast/writing-mode/box-shadow-vertical-lr.html, the shadow disappeared from the first line of text.
http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/chromium-mac-snowleopard/fast/writing-mode/box-shadow-horizontal-bt-expected.png
http://build.chromium.org/f/chromium/layout_test_results/Webkit_Mac10_6/results/layout-test-results/fast/writing-mode/box-shadow-horizontal-bt-actual.png
http://build.chromium.org/f/chromium/layout_test_results/Webkit_Mac10_6/results/layout-test-results/fast/writing-mode/box-shadow-horizontal-bt-diff.png

Was there no change for these tests on Apple Mac?

Thanks. These differences do occur in OS X WebKit, but I failed to notice them.

Filed bug 78759.

[Tony Chang]

Comment on attachment 127213 [details]
Let the background cast the shadow directly in some cases where this is possible

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2611
> +    for (const FillLayer* next = lastBackgroundLayer->next(); next; )
> +        lastBackgroundLayer = next;

This loop is bad.  We never iterate after the first iteration.  Did you mean to have next = lastBackgroundLayer->next() as the third part of the for statement?

(In reply to comment #9)
> (From update of attachment 127213 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127213&action=review
> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:2611
> > +    for (const FillLayer* next = lastBackgroundLayer->next(); next; )
> > +        lastBackgroundLayer = next;
> 
> This loop is bad.  We never iterate after the first iteration.  Did you mean to have next = lastBackgroundLayer->next() as the third part of the for statement?

Yes. Thanks for noticing it!