Bug 136136 – Implement paint flashing via GraphicsLayers in the WK2 inspector overlay

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Simon Fraser (smfr)]

Implement paint flashing via GraphicsLayers in the WK2 inspector overlay

[Simon Fraser (smfr)]

Created attachment 236943 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/18095991>

[WebKit Commit Bot]

Attachment 236943 [details] did not pass style-queue:


ERROR: Source/WebCore/WebCore.exp.in:0:  Source/WebCore/WebCore.exp.in should be sorted, use Tools/Scripts/sort-export-file script  [list/order] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 236943 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236943&action=review

> Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.cpp:54
> +    virtual void notifyAnimationEnded(const GraphicsLayer* layer, const String&)

This should have override.

> Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.h:93
> +    HashSet<WebCore::GraphicsLayer*> m_paintRectLayers;

It might be worth noting that this should use a HashSet<std::unique_ptr<WebCore::GraphicsLayer>> but can't due to HashSet issues (and point to a bug).

[Joseph Pecoraro]

Comment on attachment 236943 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236943&action=review

> Source/WebCore/inspector/InspectorInstrumentation.cpp:503
> +    if (InspectorPageAgent* pageAgent = instrumentingAgents->inspectorPageAgent()) {
> +        LayoutRect absoluteRect = LayoutRect(renderer->localToAbsoluteQuad(FloatRect(rect)).boundingBox());
> +        FrameView* view = renderer->document().view();
> +        
> +        LayoutRect rootRect = absoluteRect;
> +        if (!view->frame().isMainFrame()) {
> +            IntRect rootViewRect = view->contentsToRootView(pixelSnappedIntRect(absoluteRect));
> +            rootRect = view->frame().mainFrame().view()->rootViewToContents(rootViewRect);
> +        }
> +        pageAgent->didPaint(rootRect);
> +    }

Typically InspectorInstrumentation is just meant to be a very thin layer calling out to the Agents. Technically it could probably be auto generated. This logic could go in InspectorPageAgent. But I'm fine with it being here. We can do our own clean up later if we need to.

> Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.cpp:46
> +class RepaintIndicatorLayerClient : public GraphicsLayerClient {

Mark the class as final?

> Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.cpp:139
> +        m_page->installPageOverlay(m_paintRectOverlay, PageOverlay::FadeMode::DoNotFade);

I think m_paintRectOverlay should be uninstalled in ~WebInspectorClient. Otherwise, won't this stay around because it is RefCounted and PageOverlayController will keep a reference.

> Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.cpp:162
> +    paintLayer->addAnimation(fadeKeyframes, FloatSize(), opacityAnimation.get(), "opacity", 0);

Nit: ASCIILiteral("opacity")

> Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.h:72
> +    virtual bool overridesShowPaintRects() override { return true; }

Nit: Ideally this would be a const method.

[Simon Fraser (smfr)]

I addressed the comments.

https://trac.webkit.org/r172864