[webkit-reviews] review denied: [Bug 97801] Support invalidation tracking for composited layers : [Attachment 171709] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied vollick at chromium.org's
request for review:
Bug 97801: Support invalidation tracking for composited layers
https://bugs.webkit.org/show_bug.cgi?id=97801

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

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


This is getting close! Just a few wrinkles to work out.

> Source/WebCore/ChangeLog:8
> +	   GraphicsLayer's now store invalidated rects and can include them in

GraphicsLayers. Not possessive.

> Source/WebCore/ChangeLog:37
> +	   (WebCore):

You should delete lines like this.

> Source/WebKit/chromium/ChangeLog:8
> +	   GraphicsLayer's now store invalidated rects and can include them in

Ditto.

> Source/WebCore/page/FrameView.cpp:3658
> +void FrameView::resetTrackedRepaints()
> +{
> +    m_trackedRepaintRects.clear();
> +#if USE(ACCELERATED_COMPOSITING)
> +    if (RenderView* root = rootRenderer(this)) {
> +	   RenderLayerCompositor* compositor = root->compositor();
> +	   compositor->resetTrackedRepaintRects();
> +    }
> +#endif
> +}
> +

If you want this to work in subframes as well, this needs to traverse the frame
tree. (Or maybe that should be done up in Frame code).

> Source/WebCore/platform/graphics/GraphicsLayer.cpp:57
> +    static RepaintMap* theMap = 0;
> +    if (!theMap)
> +	   theMap = new RepaintMap();

Use Use DEFINE_STATIC_LOCAL

> Source/WebCore/platform/graphics/GraphicsLayer.cpp:559
> +    if (repaintRectMap().contains(this))
> +	   repaintRectMap().remove(this);

The contains() test is redundant.

> Source/WebCore/platform/graphics/GraphicsLayer.h:410
> +    void resetTrackedRepaints();
> +    void addRepaintRect(const FloatRect&);

Shouldn't these be protected? They are only called by subclasses. I think.

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:392
> +	   addRepaintRect(FloatRect(m_position, m_size));

I think the repaint rect should be local, so don't use m_position as the
origin.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:674
> +    addRepaintRect(rect);

setNeedsDisplay() on GraphicsLayerCA passes hugeRect:

    FloatRect hugeRect(-numeric_limits<float>::max() / 2,
-numeric_limits<float>::max() / 2,
		       numeric_limits<float>::max(),
numeric_limits<float>::max());

so this won't match other platforms. We could possibly change that to use 0,0,
m_size.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:449
> +	   addRepaintRect(FloatRect(m_position, m_size));

Ditto.

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:91
> +    addRepaintRect(FloatRect(m_position, m_size));

Ditto.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2042
> +    GraphicsLayer* graphicsLayer = backing->graphicsLayer();
> +    resetTrackedRepaintRectsRecursive(graphicsLayer);

This is missing the fact that a RenderLayerBacking owns lots of GraphicsLayers:


    OwnPtr<GraphicsLayer> m_ancestorClippingLayer; // only used if we are
clipped by an ancestor which is not a stacking context
    OwnPtr<GraphicsLayer> m_graphicsLayer;
    OwnPtr<GraphicsLayer> m_foregroundLayer;	   // only used in cases where
we need to draw the foreground separately
    OwnPtr<GraphicsLayer> m_containmentLayer; // Only used if we have clipping
on a stacking context with compositing children, or if the layer has a tile
cache.
    OwnPtr<GraphicsLayer> m_maskLayer;		   // only used if we have a
mask

    OwnPtr<GraphicsLayer> m_layerForHorizontalScrollbar;
    OwnPtr<GraphicsLayer> m_layerForVerticalScrollbar;
    OwnPtr<GraphicsLayer> m_layerForScrollCorner;

    OwnPtr<GraphicsLayer> m_scrollingLayer; // only used if the layer is using
composited scrolling.
    OwnPtr<GraphicsLayer> m_scrollingContentsLayer; // only used if the layer
is using composited scrolling.

Which do you wnat to dump repaint rects for?