[webkit-reviews] review denied: [Bug 23361] Optimize images in layer when using accelerated compositing : [Attachment 28916] initial patch for coding review

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied 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 28916: initial patch for coding review
https://bugs.webkit.org/attachment.cgi?id=28916&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> diff --git a/WebCore/rendering/RenderImage.cpp
b/WebCore/rendering/RenderImage.cpp

> +void RenderImage::notifyFinished(CachedResource* newImage)
> +{
> +    if (documentBeingDestroyed())
> +	   return;
> +
> +#if USE(ACCELERATED_COMPOSITING)
> +    if ((newImage == m_cachedImage) && layer()) {

It's slightly faster to test hasLayer(), not layer().

> +// A layer can use an inner content layer if the render layer's object is a
replaced object and has no children.
> +// This allows the GraphicsLayer to display the RenderLayer contents
directly; it's used for images and video.

Not video.

> +    // Video always uses an inner layer (for now).
> +    if (renderObject->isVideo())
> +	   return true;

Remove this.

> +	   // FIXME: video can go here once we hook up accelerated compositing
for video
> +	   /* if (renderer()->isVideo()) {
> +	       drawsContent = hasBoxDecorations ||
renderer()->style()->hasBackground();
> +	   } */

Remove this.

> +	   if (renderer()->isImage()) {
> +	       RenderImage* imageRenderer = (RenderImage*)renderer();
> +	       if (imageRenderer && imageRenderer->cachedImage() &&
imageRenderer->cachedImage()->image())
> +		  
m_graphicsLayer->setContentsToImage(imageRenderer->cachedImage()->image());
> +	       
> +	       drawsContent = false;
> +	   }
> +	   
> +	   if (rendererHasBackground()) {
> +	       m_graphicsLayer->setBackgroundColor(rendererBackgroundColor());
> +	       hasImageBackgroundColor = true;
> +	   }
> +    } else {
> +	   m_graphicsLayer->clearContents();
> +	   drawsContent = true;
> +    }
> +    
> +    if (isSimpleContainerCompositingLayer())
>	   drawsContent = false;

I don't think we need to call isSimpleContainerCompositingLayer() if we've
already determined that this
is a simple image layer.

> +void RenderLayerBacking::rendererContentChanged()
> +{
> +    bool hasBoxDecorations;
> +    if (useInnerContentLayer(hasBoxDecorations) && renderer()->isImage()) {
> +	   RenderImage* imageRenderer = (RenderImage*)renderer();
> +	   if (imageRenderer &&
> +	       imageRenderer->cachedImage() &&
> +	       imageRenderer->cachedImage()->image() &&
> +	       imageRenderer->cachedImage()->isLoaded()) {
> +	       // We have to wait until the image is fully loaded before
setting it on the layer.

Not when <rdar://problem/5798760> is fixed. We should think about whether we
want to set images
on layers while the image is being loaded.

> +	       // This is a no-op if the layer doesn't have an inner layer for
the image.
> +	      
m_graphicsLayer->setContentsToImage(imageRenderer->cachedImage()->image());
> +	       
> +	       // Image animation is "lazy", in that it automatically stops
unless someone is drawing
> +	       // the image. So we have to kick the animation each time; this
has the downside that the
> +	       // image will keep animating, even if it's layer is not visible.


its, not it's.

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

> +    // Returns true if we can optimize the RenderLayer to draw the replaced
content
> +    // directly into a compositing buffer
> +    bool useInnerContentLayer(bool& hasBoxDecorations) const;

I'm not a big fan of returning hasBoxDecorations as a side-effect of this
method. Can it just
be an accessor on the style?

> diff --git a/WebCore/rendering/RenderObject.h
b/WebCore/rendering/RenderObject.h
> index c52b1ae..11631b0 100644
> --- a/WebCore/rendering/RenderObject.h
> +++ b/WebCore/rendering/RenderObject.h
> @@ -269,6 +269,7 @@ public:
>      virtual bool isTextControl() const { return false; }
>      virtual bool isTextArea() const { return false; }
>      virtual bool isTextField() const { return false; }
> +    virtual bool isVideo() const { return false; }

I don't think we want this now.

> diff --git a/WebCore/rendering/RenderVideo.h
b/WebCore/rendering/RenderVideo.h
> index 43c1e7b..b69c1f7 100644
> --- a/WebCore/rendering/RenderVideo.h
> +++ b/WebCore/rendering/RenderVideo.h
> @@ -41,6 +41,8 @@ public:
>  
>      virtual const char* renderName() const { return "RenderVideo"; }
>  
> +    virtual bool isVideo() const { return true; }

Ditto.

r- for now, but I think it's generally OK.