Bug 129441 – Web Inspector: highlight data for overlay should use protocol type builders

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

Right now, InspectorOverlay::buildObjectForHighlightedNode and its helper methods manually build up InspectorObjects. This makes it really hard to tell what the shape of the data is without looking at the code that builds it. It also is a very thin abstraction between the code on the overlay page and its backend.

I suggest we add a new Overlay domain that contains these types. This will give us type builders for free. At a later time, we could easily convert the InspectorOverlay::evaluateInOverlay calls into protocol commands.

[Radar WebKit Bug Importer]

<rdar://problem/16186822>

[Brian Burg]

Created attachment 244843 [details]
Patch

[Timothy Hatcher]

Comment on attachment 244843 [details]
Patch

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

Looks good! There might be one issue with the ElementData change.

> Source/WebCore/inspector/InspectorController.h:55
> +namespace Protocol { namespace OverlayTypes {
> +class NodeHighlightData;
> +} }
>  }

Some newlines would help. (Unless this is the common style now.)

> Source/WebCore/inspector/InspectorOverlay.cpp:719
> -    elementInfo->setString("nodeWidth", String::number(modelObject ? adjustForAbsoluteZoom(modelObject->pixelSnappedOffsetWidth(), *modelObject) : boundingBox.width()));
> -    elementInfo->setString("nodeHeight", String::number(modelObject ? adjustForAbsoluteZoom(modelObject->pixelSnappedOffsetHeight(), *modelObject) : boundingBox.height()));
> -    
> +    auto sizeObject = Inspector::Protocol::OverlayTypes::Size::create()
> +        .setWidth(modelObject ? adjustForAbsoluteZoom(modelObject->pixelSnappedOffsetWidth(), *modelObject) : boundingBox.width())
> +        .setHeight(modelObject ? adjustForAbsoluteZoom(modelObject->pixelSnappedOffsetHeight(), *modelObject) : boundingBox.height())
> +        .release();
> +    elementData->setSize(WTF::move(sizeObject));

Doesn't this require a change in the OverlayPage script? nodeWidth and nodeHeight is now size.width and size.height, right?

> Source/WebCore/inspector/InspectorOverlay.cpp:870
> +        .setDeviceScaleFactor(m_page.deviceScaleFactor())

I'm surprised we need to pass this along.

> Source/WebCore/inspector/InspectorOverlay.h:49
> +namespace Protocol {
> +namespace OverlayTypes {
> +class NodeHighlightData;
> +}
> +}

Like this.

[Timothy Hatcher]

Comment on attachment 244843 [details]
Patch

Never mind, I didn't see the InspectorOverlayPage.js changes on the first pass.

[Brian Burg]

Created attachment 244846 [details]
Patch

[Brian Burg]

Created attachment 244847 [details]
EWS Round 2

[Brian Burg]

Created attachment 244851 [details]
EWS Round 3

[Brian Burg]

Committed r178631: <http://trac.webkit.org/changeset/178631>

[Brian Burg]

(In reply to comment #8)
> Committed r178631: <http://trac.webkit.org/changeset/178631>

Includes windows and GTK build fixes.

[Daniel Bates]

(In reply to comment #8)
> Committed r178631: <http://trac.webkit.org/changeset/178631>

This broke the iOS build.

Committed build fix in <http://trac.webkit.org/changeset/178636>.