[Webkit-unassigned] [Bug 91607] Web Inspector: Protocol Extension: add getNamedFlowCollection command

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=91607


Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #153250|review?                     |review-
               Flag|                            |




--- Comment #7 from Pavel Feldman <pfeldman at chromium.org>  2012-07-19 07:24:03 PST ---
(From update of attachment 153250)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.