Bug 136138 – Implement paint flashing in the WK1 InspectorOverlay page

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

[Simon Fraser (smfr)]

Implement paint flashing in the WK1 InspectorOverlay page

[Simon Fraser (smfr)]

Created attachment 236944 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/18096165>

[Sam Weinig]

Comment on attachment 236944 [details]
Patch

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

> Source/WebCore/inspector/InspectorOverlay.cpp:494
> +    double removeTime = currentTime() + 0.25;

Wat? Crono?

> Source/WebCore/inspector/InspectorOverlay.cpp:498
> +        m_paintRectUpdateTimer.startRepeating(0.016);

Wat?

> Source/WebCore/inspector/InspectorOverlayPage.js:210
> +    // Don't get the context until we need to paint, to avoid backing store allocation.

This comment is weirdly paragraphed?

> Source/WebCore/inspector/InspectorOverlayPage.js:216
> +    // clear paint rects?

This is weird? At least add a FIXME: and capitalize the C in clear.

> Source/WebKit/mac/WebCoreSupport/WebInspectorClient.h:107
> +#if PLATFORM(IOS)
> +    bool m_showingPaintRects;
> +#endif
>  };

This is unused. You should remove it.

[Joseph Pecoraro]

Comment on attachment 236944 [details]
Patch

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

> Source/WebCore/inspector/InspectorOverlay.cpp:347
> +    object->setNumber("x", rect.x());
> +    object->setNumber("y", rect.y());
> +    object->setNumber("width", rect.width());
> +    object->setNumber("height", rect.height());

Nit: ASCIILiteral for each of the keys.

> Source/WebCore/inspector/InspectorOverlay.cpp:528
> +    evaluateInOverlay("updatePaintRects", fragmentsArray.release());

Nit: ASCIILiteral("updatePaintRects")

> Source/WebCore/inspector/InspectorOverlayPage.js:183
> +    for (var i in paintRectList) {
> +        var rectObject = paintRectList[i];
> +        context.fillRect(rectObject.x, rectObject.y, rectObject.width, rectObject.height);
> +    }

You should not use "for..in" for array iteration (and paintRectList is an Array).

You could write this:

    for (var rectObject of paintRectList)
        context.fillRect(rectObject.x, rectObject.y, rectObject.width, rectObject.height);

> Source/WebKit/mac/WebCoreSupport/WebInspectorClient.h:80
> +    virtual bool overridesShowPaintRects() override { return true; }

Ideally this would be "const".

[Simon Fraser (smfr)]

https://trac.webkit.org/r172869