[webkit-reviews] review denied: [Bug 108203] Allow direct compositing of background images : [Attachment 187875] Patch

Fri Mar 15 11:39:01 PDT 2013

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Noam Rosenthal
<noam at webkit.org>'s request for review:
Bug 108203: Allow direct compositing of background images
https://bugs.webkit.org/show_bug.cgi?id=108203

Attachment 187875: Patch
https://bugs.webkit.org/attachment.cgi?id=187875&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=187875&action=review

> Source/WebCore/platform/graphics/GraphicsLayer.h:328
> +    // contentsTileRect is a single tile of contents in layer coordinates,
if contents is to be repeated.

This comment and name are a bit ambiguous. For repeated contents, does this
represent just a single tile (in order to specify the offset and size), not the
area covered by tiles?

Might it be clearer to use phase and tileSize instead?

> Source/WebCore/rendering/RenderLayerBacking.cpp:1344
> +    Color color = style->visitedDependentColor(CSSPropertyBackgroundColor);
> +    if (color.isValid() && color.alpha())
> +	   return false;

Why does an alpha color bail here? Can't you still set the layer's background
color and also do tiling?

> Source/WebCore/rendering/RenderLayerBacking.cpp:1353
> +    if (!styleImage->isLoaded())
> +	   return false;

Does the image loading cause us to re-evaluate compositing?

> Source/WebCore/rendering/RenderLayerBacking.cpp:1363
> +    enum { MaxSizeForPatten = 1024 };
> +    if (image->width() > MaxSizeForPatten || image->height() >
MaxSizeForPatten)
> +	   return false;

This looks pretty platform-specific.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1377
> +    if (!isSimpleContainer || !style->hasBackgroundImage()) {
> +	   m_graphicsLayer->setContentsToImage(0);
> +	   return;

It's not clear from the function name that you'll never get here for composited
<img>, so setting contents to image unconditionally looks a bit weird.

>
LayoutTests/compositing/patterns/direct-pattern-compositing-add-text-expected.h
tml:2
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
> +   "http://www.w3.org/TR/html4/loose.dtd">

Yuck.

> LayoutTests/compositing/patterns/direct-pattern-compositing-add-text.html:2
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
> +   "http://www.w3.org/TR/html4/loose.dtd">

Ditto.