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

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 97801: Support invalidation tracking for composited layers
https://bugs.webkit.org/show_bug.cgi?id=97801

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

------- Additional Comments from vollick at chromium.org
> (From update of attachment 170406 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=170406&action=review
>
> > 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.

Since setTracksRepaints now always calls resetTrackedRepaints, I think this is
now a non-issue.

>
> > 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.
Done.
>
> > Source/WebCore/platform/graphics/GraphicsLayerClient.h:88
> > +	 virtual bool isTrackingRepaints() const { return false; }
>
> I think shouldTrackRepaints() is a better name for this.
I chose this name to be consistent with FrameView::isTrackingRepaints. I'd be
happy to change this everywhere, though. Should I do 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.
I had started down this route. I had initially made
GraphicsLayer::setNeedsDisplay() non virtual and changed the implementation to
first update the list of tracked repaint rects, and then turn around and call
the pure virtual GraphicsLayer::setNeedsDisplayInternal(). Unfortunately, every
port seems to use different logic to decide whether or not it would actually
invalidate so I abandoned this approach and added the addRepaintRect calls to
the GraphicsLayer subclasses. Please let me know if I'm just misunderstanding
your suggestion.
>
> > 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.
Done.
>
> > 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.
Done.