[webkit-reviews] review denied: [Bug 97801] Make TestRunner.display() work with composited layers : [Attachment 169974] Patch

[bugzilla-daemon at webkit.org]

Sami Kyöstilä <skyostil at chromium.org> has denied  review:
Bug 97801: Make TestRunner.display() work with composited layers
https://bugs.webkit.org/show_bug.cgi?id=97801

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

------- Additional Comments from Sami Kyöstilä <skyostil at chromium.org>
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.