[webkit-reviews] review granted: [Bug 121719] Web Inspector: [CSS Regions] Display CSS Regions chain when highlighting a CSS Region node : [Attachment 212222] Patch V1

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Sep 20 15:47:01 PDT 2013


Joseph Pecoraro <joepeck at webkit.org> has granted Alexandru Chiculita
<achicu at adobe.com>'s request for review:
Bug 121719: Web Inspector: [CSS Regions] Display CSS Regions chain when
highlighting a CSS Region node
https://bugs.webkit.org/show_bug.cgi?id=121719

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

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=212222&action=review


r=me! A few optional nits, feel free to cq+.

> Source/WebCore/ChangeLog:11
> +	   When a CSS Region node is highlighted through the WebInspector, it
will also lookup all the regions
> +	   part of the same flow and inject enough information into
InspectorOverlayPage.js to get the other

Nit: "all the regions part of the same flow" => "all the regions that are part
of the same flow"

> Source/WebCore/inspector/InspectorOverlay.cpp:399
> +static void buildObjectForCSSRegionsHighlight(Node* node, InspectorObject*
nodeObject)

Nit: I think naming "nodeObject" => "highlightNodeObject" would be clearer.

> Source/WebCore/inspector/InspectorOverlayPage.js:428
> +    }
> +
> +    for (var i = 0; i < regions.length; ++i) {
> +	   var region = regions[i];

Nit: These top two loops can be combined without losing clarity. These lines
could just be removed.

> Source/WebCore/testing/Internals.cpp:746
> +    RefPtr<InspectorObject> object =
document->page()->inspectorController()->buildObjectForHighlightedNode();
> +    return object ? object->toJSONString() : String();

This JSON stringify/parse seems unnecessary. It seems the IDL for this could
return a nullable object here instead of a string
<http://www.w3.org/TR/WebIDL/#idl-object>:

    [RaisesException] object? inspectorHighlightObject(Document document);

However, having said that, I don't know what syntax would need to be here. It
might even need to be Custom in that case. So I think what you have is fine.


More information about the webkit-reviews mailing list