Bug 129441

Summary:

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

Product:

WebKit

Reporter:

Blaze Burg <bburg>

Component:

Web Inspector

Assignee:

Brian Burg <burg>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achicu, burg, dbates, graouts, joepeck, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
EWS Round 2	none
EWS Round 3	none

Blaze Burg

Reported 2014-02-27 11:25:17 PST

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.

Attachments
Patch (52.62 KB, patch) 2015-01-17 12:05 PST, Brian Burg	no flags	Details Formatted Diff Diff
Patch (54.95 KB, patch) 2015-01-17 17:02 PST, Brian Burg	no flags	Details Formatted Diff Diff
EWS Round 2 (55.52 KB, patch) 2015-01-17 17:52 PST, Brian Burg	no flags	Details Formatted Diff Diff
EWS Round 3 (56.05 KB, patch) 2015-01-17 20:32 PST, Brian Burg	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2014-02-27 11:26:02 PST

<rdar://problem/16186822>

Brian Burg

Comment 2 2015-01-17 12:05:58 PST

Created attachment 244843 [details] Patch

Timothy Hatcher

Comment 3 2015-01-17 13:09:27 PST

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 4 2015-01-17 13:10:28 PST

Comment on attachment 244843 [details] Patch Never mind, I didn't see the InspectorOverlayPage.js changes on the first pass.

Brian Burg

Comment 5 2015-01-17 17:02:27 PST

Created attachment 244846 [details] Patch

Brian Burg

Comment 6 2015-01-17 17:52:10 PST

Created attachment 244847 [details] EWS Round 2

Brian Burg

Comment 7 2015-01-17 20:32:13 PST

Created attachment 244851 [details] EWS Round 3