[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