[webkit-reviews] review denied: [Bug 103786] Use background color for GraphicsLayers when applicable : [Attachment 177049] Patch for EWS / initial review of direction

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied  review:
Bug 103786: Use background color for GraphicsLayers when applicable
https://bugs.webkit.org/show_bug.cgi?id=103786

Attachment 177049: Patch for EWS / initial review of direction
https://bugs.webkit.org/attachment.cgi?id=177049&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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.