[webkit-reviews] review denied: [Bug 91607] Web Inspector: Protocol Extension: add getNamedFlowCollection command : [Attachment 153250] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Andrei Poenaru
<poenaru at adobe.com>'s request for review:
Bug 91607: Web Inspector: Protocol Extension: add getNamedFlowCollection
command
https://bugs.webkit.org/show_bug.cgi?id=91607

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=153250&action=review


> LayoutTests/ChangeLog:3
> +	   Deleted tab space

Please remove git comments from here.

> LayoutTests/ChangeLog:16
> +	   Additional information of the change such as approach, rationale.
Please add per-function descriptions below (OOPS!).

Nuke this line

> LayoutTests/inspector/styles/protocol-getNamedFlowCollection-command.html:25
> +    function test() {

You probably should pick another name in order to not reuse "test"

> LayoutTests/inspector/styles/protocol-getNamedFlowCollection-command.html:26
> +	   function namedFlowCallback(namedFlows) {

Please place { on the next line

> LayoutTests/inspector/styles/protocol-getNamedFlowCollection-command.html:31
> +		   InspectorTest.completeTest();

missing return; below.

> LayoutTests/inspector/styles/protocol-getNamedFlowCollection-command.html:42
> +	   function documentCallback(document) {

{ on the next line.

> Source/WebCore/ChangeLog:2
> +	   Web Inspector: Protocol Extension: add getNamedFlowCollection
command

missing blank line above

> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:48
> +    return m_namedFlows;

Exposing private member for read-write does not sound good.

> Source/WebCore/inspector/Inspector.json:2294
> +		       { "name": "documentNodeId", "$ref": "DOM.NodeId",
"description": "The document node id for which to get the Named Flow
Collection."}

we typically call node ids "nodeId"

> Source/WebCore/inspector/InspectorCSSAgent.cpp:796
> +    if (!node || !(node->isDocumentNode())) {

You should add InspectorDOMNode::assertDocumentNode instead.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:804
> +    WebKitNamedFlowCollection::NamedFlowSet& namedFlowSet =
document->namedFlows()->namedFlows();

So you could make flow collection return array of strings instead of exposing
the private object.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:807
> +	   if ((*it)->flowState() == WebKitNamedFlow::FlowStateNull)

This check could also be a part of the collection itself.