[webkit-reviews] review denied: [Bug 69441] [chromium] Keep track of the paint rect in layers : [Attachment 111114] Added update rect to CCLayerImpl also

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Shawn Singh
<shawnsingh at chromium.org>'s request for review:
Bug 69441: [chromium] Keep track of the paint rect in layers
https://bugs.webkit.org/show_bug.cgi?id=69441

Attachment 111114: Added update rect to CCLayerImpl also
https://bugs.webkit.org/attachment.cgi?id=111114&action=review

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


I think the updateRect for tiled layers is wrong - shouldn't it be in layer
space? The easiest way to check this is to see what the expected and actual
update rectangle are for an animated image layer that's scaled in CSS.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:211
> +    m_updateRect = FloatRect(m_paintRect);

m_paintRect is in content space, not layer space. Is that what we expect? All
of the other m_updateRects are in layer space, not content space.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:251
> +    // Reset any state that should be cleared for the next update.
> +    m_paintRect = IntRect();

Why do we clear the paint rect here? We didn't push that to the CCLayerImpl

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:350
> +void TiledLayerChromium::prepareToUpdate(const IntRect& visibleContentRect)

Now that the parameter name and member variable have nearly the same name
(modulo the "m_") I find this function much harder to read. I think the
original name was actually better - the caller was requesting that we prepare
to update the region corresponding with a given content rect. The fact that the
content rect represents a visible area is the caller's concern, not the concern
of this function - and in fact when we start doing tile prerendering the
passed-in content rect will not necessarily have much to do with what's
currently visible.

I think the parameter name 'contentRect' was better and the member variable
should be something like 'requestedUpdateRect' to reflect that we might not end
up actually updating that exact rectangle.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:69
>      // Prepare data needed to update textures that intersect with
contentRect.
> -    void prepareToUpdate(const IntRect& contentRect);
> -    // Update invalid textures that intersect with contentRect provided in
prepareToUpdate().
> -    void updateRect(GraphicsContext3D*, LayerTextureUpdater*);
> +    void prepareToUpdate(const IntRect& visibleContentRect);

The comment and signature are out of sync