[webkit-reviews] review granted: [Bug 23361] Optimize images in layer when using accelerated compositing : [Attachment 28927] updated patch (with changelog and manual test)

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dean Jackson
<dino at apple.com>'s request for review:
Bug 23361: Optimize images in layer when using accelerated compositing
https://bugs.webkit.org/show_bug.cgi?id=23361

Attachment 28927: updated patch (with changelog and manual test)
https://bugs.webkit.org/attachment.cgi?id=28927&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 3c2a777..6529e61 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,35 @@
> +2009-03-25  Dean Jackson  <dino at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).

Normally the bug URL goes right here on a line by itself.

> diff --git a/WebCore/manual-tests/resources/simple_image.png
b/WebCore/manual-tests/resources/simple_image.png

I think these should be pixel tests, though I'm not sure where they should
live. They could go anywhere,
since they will look OK with accel-comp turned off, and it's fine to put a
dummy 3d transform in the css.

> diff --git a/WebCore/rendering/RenderLayer.cpp
b/WebCore/rendering/RenderLayer.cpp
> index 6ca9174..6861016 100644
> --- a/WebCore/rendering/RenderLayer.cpp
> +++ b/WebCore/rendering/RenderLayer.cpp
> @@ -224,6 +224,15 @@ RenderLayerCompositor* RenderLayer::compositor() const
>      ASSERT(renderer()->view());
>      return renderer()->view()->compositor();
>  }
> +    
> +void RenderLayer::rendererContentChanged()
> +{
> +    if (compositor()->updateLayerCompositingState(this,
StyleDifferenceEqual))
> +	   compositor()->setCompositingLayersNeedUpdate();     // FIXME: layers
will only get updated after the next layout.

I'm not sure that updateLayerCompositingState() here can ever cause the layer
to move
between compositing and non-compositing, so the
setCompositingLayersNeedUpdate() may not
be required.

> diff --git a/WebCore/rendering/RenderLayerBacking.cpp
b/WebCore/rendering/RenderLayerBacking.cpp

> +    // Reject anything that has a border, a border-radius, outline, margin
or padding.
> +    bool hasBoxDecorations = hasBorderOutlineOrShadow(style) ||
style->hasMargin() || style->hasPadding();
> +    
> +    // Only optimize images that are unadorned.
> +    if (renderObject->isImage())
> +	   return !hasBoxDecorations;

No point computing hasBoxDecorations if you are not an image.

> @@ -572,9 +602,36 @@ void RenderLayerBacking::detectDrawingOptimizations()
>  {
>      bool drawsContent = true;
>  
> -    if (isSimpleContainerCompositingLayer() || paintingGoesToWindow())
> +    // Check if a replaced layer can be further simplified.
> +    bool hasImageBackgroundColor = false;
> +    if (canUseInnerContentLayer()) {
> +	   if (renderer()->isImage()) {
> +	       RenderImage* imageRenderer = (RenderImage*)renderer();
> +	       if (imageRenderer && imageRenderer->cachedImage() &&
imageRenderer->cachedImage()->image())
> +		  
m_graphicsLayer->setContentsToImage(imageRenderer->cachedImage()->image());

Hmm, this might set the image before it's fully loaded. Maybe we should just
call
rendererContentChanged() here, to avoid code duplication.

> +	       drawsContent = false;
> +	   }
> +	   
> +	   if (rendererHasBackground()) {
> +	       m_graphicsLayer->setBackgroundColor(rendererBackgroundColor());
> +	       hasImageBackgroundColor = true;
> +	   }
> +    } else {
> +	   m_graphicsLayer->clearContents();
> +	   drawsContent = true;
> +    }
> +    
> +    if (isSimpleContainerCompositingLayer())
>	   drawsContent = false;

I think this should be an else if, since you can't have
canUseInnerContentLayer() and
isSimpleContainerCompositingLayer().

> -
> +    else if (!hasImageBackgroundColor) {
> +	   // Clear the background color in case we are swapping away from a
simple layer.
> +	   m_graphicsLayer->clearBackgroundColor();
> +    }

The logic flow here is confusing. Maybe it should be:

if (canUseInnerContentLayer()) {

} else {
    if (isSimpleContainerCompositingLayer())
	...
    else if (!hasImageBackgroundColor)
	...
}

r=me with the test in a better place, and the suggestions above.