Bug 97801

I think we should deprecate display(). I think we should add: internals.startTrackingRepaints() internals.stopTrackingRepaints() ClientRectList internals.clientRepaintRects(); or something like that.

(In reply to comment #1) > I think we should deprecate display(). I think we should add: > > internals.startTrackingRepaints() > internals.stopTrackingRepaints() > ClientRectList internals.clientRepaintRects(); > > or something like that. Do you mean the visual display of repaint rectangles would go away completely and these would be dumped in text form? I think the visual display is handy for figuring out what changed, but it's also a maintenance burden. Also, in bug 99605 I considered making that a list of absolute FloatQuads to support transforms.

Having access to the list of repaint rects would also let us detect when we've repainted a region multiple times, which would be nice.

(In reply to comment #2) > Do you mean the visual display of repaint rectangles would go away completely and these would be dumped in text form? I think the visual display is handy for figuring out what changed, but it's also a maintenance burden. It's nice to have the visual display, but the main objective would be to allow for testing repaints without doing pixel tests. > Also, in bug 99605 I considered making that a list of absolute FloatQuads to support transforms. Sounds good, but you'd have to find a way to expose quads to JS. Simon

Created attachment 169439 [details] Patch This patch switches to tracking absolute float quads for the transformed paint rects. It also adds tracking for composited layers (and the non-composited content host). It is still missing tests, and does not tackle the problem of moving away from display() and pixel-based repaint testing to the text-based approach mentioned in earlier comments. Could this possibly be done in a separate patch? Note: this patch builds on https://bugs.webkit.org/show_bug.cgi?id=99605.

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Comment on attachment 169439 [details] Patch Attachment 169439 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14408020

Comment on attachment 169439 [details] Patch Attachment 169439 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14412652

Comment on attachment 169439 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169439&action=review > Tools/DumpRenderTree/chromium/TestShell.cpp:630 > + m_webViewHost->paintTrackedRepaintRectangleOverlay(); This is a bug from my patch; here (or in WebViewHost) we need to check whether we're actually tracking repaints. Otherwise all non-repaint tracking tests will end up gray.

(In reply to comment #4) > (In reply to comment #2) > > Do you mean the visual display of repaint rectangles would go away completely and these would be dumped in text form? I think the visual display is handy for figuring out what changed, but it's also a maintenance burden. > > It's nice to have the visual display, but the main objective would be to allow for testing repaints without doing pixel tests. > This is somewhat crazy but I'd like to extend our test tools (maybe garden-o-matic) to try to visualize the rects from the dumped text info. That'd be best of both worlds - easy to maintain, easy to visually tell WTF is going on in at a glance. > > Also, in bug 99605 I considered making that a list of absolute FloatQuads to support transforms. > > Sounds good, but you'd have to find a way to expose quads to JS. > > Simon

Comment on attachment 169439 [details] Patch Attachment 169439 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14400905

Comment on attachment 169439 [details] Patch Attachment 169439 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14465073

Comment on attachment 169439 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169439&action=review > Source/WebCore/page/FrameView.h:527 > - Vector<IntRect> m_trackedRepaintRects; > + Vector<FloatQuad> m_trackedTransformedRepaintRects; I don't see any benefit from having 'transformed' in the name. 'absolute' would be more descriptive. > Source/WebCore/rendering/RenderLayerBacking.cpp:1490 > + FrameView* frameView = renderer()->frame()->view(); > + if (frameView->isTrackingRepaints()) > + frameView->addTransformedRepaintRect(renderer()->localToAbsoluteQuad(FloatRect(clip), SnapOffsetForTransforms)); > + This is the wrong place for this. This isn't a "repaint". You need to move this code to RenderLayerBacking::setContentsNeedDisplay() and RenderLayerBacking::setContentsNeedDisplayInRect().

Comment on attachment 169439 [details] Patch Attachment 169439 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14465102 New failing tests: compositing/checkerboard.html compositing/layer-creation/fixed-position-and-transform.html animations/3d/matrix-transform-type-animation.html animations/3d/state-at-end-event-transform.html animations/additive-transform-animations.html http/tests/images/jpg-img-partial-load.html animations/3d/replace-filling-transform.html animations/opacity-transform-animation.html compositing/clip-change.html compositing/images/content-image-change.html http/tests/images/jpeg-partial-load.html animations/cross-fade-webkit-mask-box-image.html animations/3d/change-transform-in-end-event.html animations/suspend-transform-animation.html animations/missing-values-last-keyframe.html compositing/absolute-position-changed-with-composited-parent-layer.html compositing/absolute-position-changed-in-composited-layer.html compositing/images/direct-svg-image.html compositing/geometry/horizontal-scroll-composited.html compositing/images/direct-image-background-color.html animations/cross-fade-list-style-image.html compositing/masks/layer-mask-placement.html animations/missing-values-first-keyframe.html compositing/geometry/foreground-offset-change.html animations/cross-fade-background-image.html compositing/iframes/fixed-position-iframe.html animations/cross-fade-border-image-source.html compositing/masks/direct-image-mask.html animations/cross-fade-webkit-mask-image.html animations/state-at-end-event.html

(In reply to comment #13) > > Source/WebCore/rendering/RenderLayerBacking.cpp:1490 > > + FrameView* frameView = renderer()->frame()->view(); > > + if (frameView->isTrackingRepaints()) > > + frameView->addTransformedRepaintRect(renderer()->localToAbsoluteQuad(FloatRect(clip), SnapOffsetForTransforms)); > > + > > This is the wrong place for this. This isn't a "repaint". You need to move this code to RenderLayerBacking::setContentsNeedDisplay() and RenderLayerBacking::setContentsNeedDisplayInRect(). I'm curious about this. This is where I'd originally stuck the code to grab the repaint rects, but I hit a snag. I'm particularly interested in tracking repaints that are caused by composited scrolling. On the composited scrolling path, we eventually call GraphicsLayer::setOffsetFromRenderer, which results in a call to GraphicsLayer::setNeedsDisplay. I notice that the RenderLayerBacking::setContentsNeedDisplay*() functions turn around and call setNeedsDisplay on the appropriate graphics layers, but it seems in the case of setOffsetFromRenderer, we're able to skip past the RLB functions (and the repaint tracking) and get right to GraphicsLayer::setNeedsDisplay and cause invalidations. I'm sure that RLB::paintIntoLayer isn't the right place to add the tracking code, but I'm having a tough time finding a good spot where I can catch every repaint. Perhaps in GraphicsLayer::setNeedsDisplay and GraphicsLayer::setNeedsDisplayInRect, the graphics layers could pass the repaint rect to a new GraphicsLayerClient function (GLC::addRepaintRect(...), maybe) so that we can be sure we never miss one?

(In reply to comment #15) > (In reply to comment #13) > > > Source/WebCore/rendering/RenderLayerBacking.cpp:1490 > > > + FrameView* frameView = renderer()->frame()->view(); > > > + if (frameView->isTrackingRepaints()) > > > + frameView->addTransformedRepaintRect(renderer()->localToAbsoluteQuad(FloatRect(clip), SnapOffsetForTransforms)); > > > + > > > > This is the wrong place for this. This isn't a "repaint". You need to move this code to RenderLayerBacking::setContentsNeedDisplay() and RenderLayerBacking::setContentsNeedDisplayInRect(). > > I'm curious about this. This is where I'd originally stuck the code to grab the repaint rects, but I hit a snag. I'm particularly interested in tracking repaints that are caused by composited scrolling. On the composited scrolling path, we eventually call GraphicsLayer::setOffsetFromRenderer, which results in a call to GraphicsLayer::setNeedsDisplay. I notice that the RenderLayerBacking::setContentsNeedDisplay*() functions turn around and call setNeedsDisplay on the appropriate graphics layers, but it seems in the case of setOffsetFromRenderer, we're able to skip past the RLB functions (and the repaint tracking) and get right to GraphicsLayer::setNeedsDisplay and cause invalidations. Yeah, this will miss the scrolling ones. > > I'm sure that RLB::paintIntoLayer isn't the right place to add the tracking code, but I'm having a tough time finding a good spot where I can catch every repaint. Perhaps in GraphicsLayer::setNeedsDisplay and GraphicsLayer::setNeedsDisplayInRect, the graphics layers could pass the repaint rect to a new GraphicsLayerClient function (GLC::addRepaintRect(...), maybe) so that we can be sure we never miss one? That would be an OK way to fix this.

Comment on attachment 169439 [details] Patch Attachment 169439 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14456448

Created attachment 169974 [details] Patch

Comment on attachment 169974 [details] Patch Attachment 169974 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14496383

Comment on attachment 169974 [details] Patch Attachment 169974 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14482668

Comment on attachment 169974 [details] Patch Attachment 169974 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14486642

Comment on attachment 169974 [details] Patch Attachment 169974 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14492598

Comment on attachment 169974 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169974&action=review Neat, I like the idea of dumping the paint rectangles as a part of the layer tree. > Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.h:96 > +protected: This doesn't seem to match the cpp file. > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:456 > + m_trackedRepaintRects.append(FloatRect( It seems like LinkHighlight::updateGeometry() would be a better place for this because the underlying graphics layer isn't invalidated unconditionally. > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:476 > + m_trackedRepaintRects.append(FloatRect( Ditto for LinkHighlight::updateGeometry(). > Source/WebCore/rendering/RenderLayerBacking.cpp:1570 > + return renderer()->frame()->view()->isTrackingRepaints(); I think either frame() or view() might be null here. > Source/WebCore/rendering/RenderLayerCompositor.cpp:1989 > + for (size_t i = 0; i < graphicsLayer->children().size(); ++i) We should also walk mask and replica layers here. > Source/WebCore/rendering/RenderLayerCompositor.cpp:2019 > +bool RenderLayerCompositor::isTrackingRepaints() const It seems like nobody is calling this (and by extension RenderLayerBacking::isTrackingRepaints) so we could get rid of both? > Source/WebCore/testing/Internals.cpp:1375 > +void Internals::startTrackingRepaints(Document* document, ExceptionCode& ec) Since we're poking at FrameView/RLC directly here, do we need the WebView API changes at all? They seem kind of unnecessary because you can't retrieve the repaint rectangles through that API anyway. > Source/WebCore/testing/Internals.cpp:1390 > + RenderLayerCompositor* compositor = renderView->compositor(); Nit: FrameView already knows about RenderLayerCompositor, so it might as well do this internally in resetTrackedRepaints(). > Source/WebKit/chromium/src/WebViewImpl.cpp:448 > + , m_isTrackingRepaints(false) Not used? > Source/WebKit/chromium/src/WebViewImpl.cpp:1836 > + if (!isAcceleratedCompositingActive()) Again, FrameView could do the rest internally. > Source/WebKit/chromium/src/WebViewImpl.h:880 > + Vector<WebFloatQuad> m_trackedTransformedRepaintRects; Not used? > LayoutTests/compositing/repaint/invalidations-on-composited-layers.html:4 > +This test checks that the contents of accelerated scrolling layers are properly This comment seems out of date. > LayoutTests/compositing/repaint/invalidations-on-composited-layers.html:18 > + background: blue; Some space/tab weirdness going on here? > LayoutTests/compositing/repaint/invalidations-on-composited-layers.html:34 > + if (!window.testRunner) { Might want to check for window.internals too.

Comment on attachment 169974 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169974&action=review > Source/WebCore/ChangeLog:10 > + Allows tracking of graphics layer invalidations. The graphics layers > + can now store (local) invalidation rects and include them in the > + layer tree dump. It's not clear to me that this is sufficiently better than just tracking absolute repaint rects on FrameView to justify the extra overhead of having each GraphicsLayer track repaint rects. It's worse in the sense that you can't easily draw the repaints rects into an overlay, and that existing repaint tests with compositing layers won't work unless changed to use layerTreeAsText() etc. > LayoutTests/compositing/repaint/invalidations-on-composited-layers.html:55 > + text.innerHTML = window.internals.layerTreeAsText(document); It's better to use a <pre> to preserve whitespace and wrapping.

Created attachment 170231 [details] Patch (In reply to comment #23) > (From update of attachment 169974 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169974&action=review > > Neat, I like the idea of dumping the paint rectangles as a part of the layer tree. > > > Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.h:96 > > +protected: > > This doesn't seem to match the cpp file. Leftovers from a failed approach. Removed. > > > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:456 > > + m_trackedRepaintRects.append(FloatRect( > > It seems like LinkHighlight::updateGeometry() would be a better place for this because the underlying graphics layer isn't invalidated unconditionally. Good idea. Done. > > > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:476 > > + m_trackedRepaintRects.append(FloatRect( > > Ditto for LinkHighlight::updateGeometry(). Done. > > > Source/WebCore/rendering/RenderLayerBacking.cpp:1570 > > + return renderer()->frame()->view()->isTrackingRepaints(); > > I think either frame() or view() might be null here. Changed to something hopefully more robust. > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1989 > > + for (size_t i = 0; i < graphicsLayer->children().size(); ++i) > > We should also walk mask and replica layers here. Done. > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:2019 > > +bool RenderLayerCompositor::isTrackingRepaints() const > > It seems like nobody is calling this (and by extension RenderLayerBacking::isTrackingRepaints) so we could get rid of both? GraphicsLayers check if their GraphicsLayerClient is tracking repaints, so I think we need this for classes that implement GraphicsLayerClient. > > > Source/WebCore/testing/Internals.cpp:1375 > > +void Internals::startTrackingRepaints(Document* document, ExceptionCode& ec) > > Since we're poking at FrameView/RLC directly here, do we need the WebView API changes at all? They seem kind of unnecessary because you can't retrieve the repaint rectangles through that API anyway. Only isTrackingRepaints, I think. This is how the NonCompositedContentHost (a GraphicsLayerClient) checks if we're tracking repaints. > > > Source/WebCore/testing/Internals.cpp:1390 > > + RenderLayerCompositor* compositor = renderView->compositor(); > > Nit: FrameView already knows about RenderLayerCompositor, so it might as well do this internally in resetTrackedRepaints(). Good idea. Done. > > > Source/WebKit/chromium/src/WebViewImpl.cpp:448 > > + , m_isTrackingRepaints(false) > > Not used? Removed. > > > Source/WebKit/chromium/src/WebViewImpl.cpp:1836 > > + if (!isAcceleratedCompositingActive()) > > Again, FrameView could do the rest internally. Removed. > > > Source/WebKit/chromium/src/WebViewImpl.h:880 > > + Vector<WebFloatQuad> m_trackedTransformedRepaintRects; > > Not used? Removed. > > > LayoutTests/compositing/repaint/invalidations-on-composited-layers.html:4 > > +This test checks that the contents of accelerated scrolling layers are properly > > This comment seems out of date. Fixed. > > > LayoutTests/compositing/repaint/invalidations-on-composited-layers.html:18 > > + background: blue; > > Some space/tab weirdness going on here? Yeah, my editor got confused. Fixed. > > > LayoutTests/compositing/repaint/invalidations-on-composited-layers.html:34 > > + if (!window.testRunner) { > > Might want to check for window.internals too. Done.

Comment on attachment 170231 [details] Patch Attachment 170231 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14496817

I think tracking per-GraphicsLayer provides a lot of value relative to smooshing everything on to the FrameView. Consider the case of a compositor layer scrolling down over a heavy background. Ideally, we would have invalidations only on the scrolling GraphicsLayer and not on the FrameView underneath. If we collapsed it all down to the FrameView, we wouldn't be able to tell what was going on. The visualization question is somewhat separate - we can visualize on a per-GraphicsLayer basis if we really want to but I think the maintenance overhead of having pixel tests for repaint costs us far more than we gain. How often are the mac pixel baselines for fast/repaint up-to-date enough to be able to spot regressions on the mac port? In my experience, they basically never are up to date. Text baselines would be far easier to maintain and thus provide much better coverage in practice on the bots and the cost of making it slightly harder in some cases to understand failures. Ideally, we would generally not be creating new failures so I think optimizing for better coverage is the right way to go.

Comment on attachment 170231 [details] Patch Attachment 170231 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14532057 New failing tests: compositing/repaint/invalidations-on-composited-layers.html

Created attachment 170380 [details] Patch

Created attachment 170406 [details] Patch Sorry for the spam. I'd missed Simon's comment about using <pre> in the layout tests -- I've made that change in this patch.

Comment on attachment 170406 [details] Patch I think this is good - what do you think, Simon?

Comment on attachment 170406 [details] Patch 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.

Comment on attachment 170406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170406&action=review I like the way this looks. Thanks for doing this. >> Source/WebCore/page/FrameView.h:342 >> 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. Seems a little redundant to have both setTracksRepaints(..., ResetTrackedRepaints) and resetTrackedRepaints(). Maybe we only need one way to reset? > LayoutTests/compositing/repaint/invalidations-on-composited-layers.html:36 > + alert('This test requires window.interals to run!'); Typo: interals.

Comment on attachment 170406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170406&action=review > Source/WebCore/page/FrameView.cpp:3669 > + ts << "(repaint rects\n"; Could check whether the list of rects is empty here for consistency with GraphicsLayers.

Created attachment 171685 [details] Patch > (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.

Comment on attachment 171685 [details] Patch Attachment 171685 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14625969

Created attachment 171709 [details] Patch Exporting FrameView::resetRepaintRects to fix mac build.

Comment on attachment 171709 [details] Patch 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?

Created attachment 171890 [details] Patch (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.

Comment on attachment 171890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171890&action=review > Source/WebCore/rendering/RenderLayerCompositor.cpp:2066 > +bool RenderLayerCompositor::isTrackingRepaints() const > +{ > + FrameView* frameView = m_renderView ? m_renderView->frameView() : 0; > + if (!frameView) > + return false; > + FrameView* rootFrameView = frameView->frame()->tree()->top()->view(); > + if (!rootFrameView) > + return false; > + return rootFrameView->isTrackingRepaints(); > +} This code is going to run in release builds. I wonder if we should cache the flag in RLC?

Created attachment 171916 [details] Patch (In reply to comment #40) > (From update of attachment 171890 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171890&action=review > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:2066 > > +bool RenderLayerCompositor::isTrackingRepaints() const > > +{ > > + FrameView* frameView = m_renderView ? m_renderView->frameView() : 0; > > + if (!frameView) > > + return false; > > + FrameView* rootFrameView = frameView->frame()->tree()->top()->view(); > > + if (!rootFrameView) > > + return false; > > + return rootFrameView->isTrackingRepaints(); > > +} > > This code is going to run in release builds. I wonder if we should cache the flag in RLC? Sounds good. Done. I've put another r? on this in case I've gone about this the wrong way.

Comment on attachment 171916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171916&action=review > Source/WebCore/page/FrameView.cpp:3650 > +static void setTracksRepaintsRecursive(Frame* frame, bool trackRepaints) > +{ > + if (!frame) > + return; > + > + if (RenderView* root = frame->contentRenderer()) > + root->compositor()->setTracksRepaints(trackRepaints); > + > + for (unsigned i = 0; i < frame->tree()->childCount(); ++i) > + setTracksRepaintsRecursive(frame->tree()->child(i), trackRepaints); > +} You can do this non-recursively; the FrameTree can enumerate all subframes for you.

Created attachment 171917 [details] Patch (In reply to comment #42) > (From update of attachment 171916 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171916&action=review > > > Source/WebCore/page/FrameView.cpp:3650 > > +static void setTracksRepaintsRecursive(Frame* frame, bool trackRepaints) > > +{ > > + if (!frame) > > + return; > > + > > + if (RenderView* root = frame->contentRenderer()) > > + root->compositor()->setTracksRepaints(trackRepaints); > > + > > + for (unsigned i = 0; i < frame->tree()->childCount(); ++i) > > + setTracksRepaintsRecursive(frame->tree()->child(i), trackRepaints); > > +} > > You can do this non-recursively; the FrameTree can enumerate all subframes for you. Done.

Comment on attachment 171917 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171917&action=review > Source/WebCore/platform/graphics/GraphicsLayer.cpp:49 > +namespace { I don't think there's any benefit having this in an anonymous namespace.

Created attachment 172104 [details] Patch for landing

Comment on attachment 172104 [details] Patch for landing Clearing flags on attachment: 172104 Committed r133332: <http://trac.webkit.org/changeset/133332>

All reviewed patches have been landed. Closing bug.