[webkit-reviews] review denied: [Bug 97801] Support invalidation tracking for composited layers : [Attachment 170406] 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 170406: Patch
https://bugs.webkit.org/attachment.cgi?id=170406&action=review

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


Seems good. But I'd like to see a few things:
1. Do the FrameView part in isolation first, and convert a few repaint tests to
use it.
2. Do the chromium parts in a separate patch.

> Source/WebCore/page/FrameView.h:342
> +    void setTracksRepaints(bool, ResetTrackedRepaintsBehavior =
ResetTrackedRepaints);
>      bool isTrackingRepaints() const { return m_isTrackingRepaints; }
>      void resetTrackedRepaints() { m_trackedRepaintRects.clear(); }

Why doesn't resetTrackedRepaints() call setTracksRepaints(false,
ResetTrackedRepaints)? Right now you have a bug where resetTrackedRepaints()
doesn't call into the compositor.

> Source/WebCore/platform/graphics/GraphicsLayer.h:506
> +    Vector<FloatRect> m_trackedRepaintRects; // Used for debugging.

Let's make this a Vector* to avoid wasting space for end users. In fact, maybe
we should use a singleton HashMap, since we really don't want to waste space
for a testing-only feature.

> Source/WebCore/platform/graphics/GraphicsLayerClient.h:88
> +    virtual bool isTrackingRepaints() const { return false; }

I think shouldTrackRepaints() is a better name for this.

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:395
>  void GraphicsLayerBlackBerry::setNeedsDisplay()
>  {
> -    if (drawsContent())
> +    if (drawsContent()) {
>	   m_layer->setNeedsDisplay();
> +
> +	   addRepaintRect(FloatRect(m_position, m_size));
> +    }

Rather than require every port to do this, maybe make
GraphicsLayer::setNeedsDisplay() etc non-virtual and require overrides call the
base class method.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1155
> +    // The true root layer is not included in the dump, so if we want to
report
> +    // its repaint rects, they must be appended here.
> +    if (flags & LayerTreeFlagsIncludeRepaintRects)
> +	  
layerTreeText.append(m_renderView->frameView()->trackedRepaintRectsAsText());

I think this should be a prepend, since we dump layers starting at the root.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2046
> +bool RenderLayerCompositor::isTrackingRepaints() const
> +{
> +    RenderLayerBacking* backing = rootRenderLayer()->backing();
> +    if (!backing)
> +	   return false;
> +
> +    return backing->isTrackingRepaints();
> +}

This is backwards from how we normally handle the fact that both
RenderLayerBacking and RenderLayerCompositor are GraphicsLayerClients. You
should move the code from RenderLayerBacking::isTrackingRepaints() to here, and
have RLB just call the compositor method.