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

------- Additional Comments from vollick at chromium.org
(In reply to comment #38)
> (From update of attachment 171709 [details])
> 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.
Done.
>
> > Source/WebCore/ChangeLog:37
> > +	     (WebCore):
>
> You should delete lines like this.
Done.
>
> > Source/WebKit/chromium/ChangeLog:8
> > +	     GraphicsLayer's now store invalidated rects and can include them
in
>
> Ditto.
Done.
>
> > 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).
As I mucked with subframes, it looked like repaint rects were only being stored
on the root frame view, and if the subframes were composited, their graphics
layers appeared to be part of the larger graphics layer tree.
It seems like clearing this list of rects and walking the graphics layer tree
will catch all tracked repaint rects. Please let me know if that's wrong.
>
> > Source/WebCore/platform/graphics/GraphicsLayer.cpp:57
> > +	 static RepaintMap* theMap = 0;
> > +	 if (!theMap)
> > +	     theMap = new RepaintMap();
>
> Use Use DEFINE_STATIC_LOCAL
Done.
>
> > Source/WebCore/platform/graphics/GraphicsLayer.cpp:559
> > +	 if (repaintRectMap().contains(this))
> > +	     repaintRectMap().remove(this);
>
> The contains() test is redundant.
Removed.
>
> > 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.
They're needed by LinkHighlight which causes invalidations in an unusual way.
>
> > 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.
Fixed.
>
> > 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.
I've made it so we clip the repaint rects before storing them. Is that ok?
>
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:449
> > +	     addRepaintRect(FloatRect(m_position, m_size));
>
> Ditto.
Fixed.
>
> > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:91
> > +	 addRepaintRect(FloatRect(m_position, m_size));
>
> Ditto.
Fixed.
>
> > 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?

This was a mistake -- I've changed the code to start recurring from the root
graphics layer as is done elsewhere in RLC.