[webkit-reviews] review denied: [Bug 63760] [chromium] Compositor must reserve all textures before drawing : [Attachment 99387] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Vangelis Kokkevis
<vangelis at chromium.org>'s request for review:
Bug 63760: [chromium] Compositor must reserve all textures before drawing
https://bugs.webkit.org/show_bug.cgi?id=63760

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=99387&action=review

I'm pretty sure this is gonna make us re-upload images on every frame, so r-
for that.  Otherwise this seems good but I have questions about some other
bits.

> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:162
> -	   m_tiler->prepareToUpdate(paintRect, m_textureUpdater.get());
>      }
> +    IntRect layerRect = visibleLayerRect(targetSurfaceRect);
> +    if (layerRect.isEmpty())
> +	   return;
> +
> +    m_tiler->prepareToUpdate(layerRect, m_textureUpdater.get());

by moving this out of the if check won't this reupload the entire image layer
every frame?  that seems bad...

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:406
> +		   IntRect scissorRect = layer->ccLayerImpl()->scissorRect();
>		   targetSurfaceRect.intersect(scissorRect);

nit: you don't really need a scissorRect local any more

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-974
> -    bool isLayerVisible = targetSurfaceRect.intersects(layerRect);
> -    if (!isLayerVisible) {
> -	   layer->unreserveContentsTexture();
> -	   return;

is this early out not useful any more, not valid, or something else?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1073
> +

nit: unnecessary whitespace change here