Web Inspector: Protocol Extension: add getNamedFlowCollection command

[Andrei Poenaru]

Implement the getNamedFlowCollection command from this proposed protocol extension:

https://bug-90789-attachments.webkit.org/attachment.cgi?id=152527

[Vivek Galatage]

Are these all proposed APIs belong to public inspector protocol APIs? Or can some of them be private APIs?

[Andrei Poenaru]

(In reply to comment #1)
> Are these all proposed APIs belong to public inspector protocol APIs? Or can some of them be private APIs?

Does this mean if they will have "hidden": true?  If yes, then they will be marked as "hidden".

[Vivek Galatage]

(In reply to comment #2)
> (In reply to comment #1)
> > Are these all proposed APIs belong to public inspector protocol APIs? Or can some of them be private APIs?
> 
> Does this mean if they will have "hidden": true?  If yes, then they will be marked as "hidden".

Yes that's exactly my question was. Thank you.

[Andrei Poenaru]

Created attachment 153016 [details]
Patch

[Pavel Feldman]

Comment on attachment 153016 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153016&action=review

> Source/WebCore/inspector/InspectorCSSAgent.cpp:794
> +    Document* document = m_domAgent->document();

You are currently getting flows from the main frame only. What if the page uses iframes or frame sets? You probably should pass context node id (from DOMAgent terms) into this method to get the flows for a given document.

[Andrei Poenaru]

Created attachment 153250 [details]
Patch

[Pavel Feldman]

Comment on attachment 153250 [details]
Patch

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.

[Andrei Poenaru]

(In reply to comment #7)
> > Source/WebCore/inspector/InspectorCSSAgent.cpp:796
> > +    if (!node || !(node->isDocumentNode())) {
> 
> You should add InspectorDOMNode::assertDocumentNode instead.

I can't find "InspectorDOMNode". Did you meant "Node"?

[Pavel Feldman]

(In reply to comment #8)
> (In reply to comment #7)
> > > Source/WebCore/inspector/InspectorCSSAgent.cpp:796
> > > +    if (!node || !(node->isDocumentNode())) {
> > 
> > You should add InspectorDOMNode::assertDocumentNode instead.
> 
> I can't find "InspectorDOMNode". Did you meant "Node"?

Sorry, I meant introduce InspectorDOMAgent::assertDocument as in InspectorDOMAgent::assertNode / InspectorDOMAgent::assertElement.

[Andrei Poenaru]

Created attachment 153444 [details]
Patch

[Pavel Feldman]

Comment on attachment 153444 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153444&action=review

> Source/WebCore/dom/WebKitNamedFlowCollection.h:33
> +#include <InspectorBackendDispatcher.h>

You should not make arbitrary component depend on the inspector guts. Also, these are not visible in case of !ENABLED(INSPECTOR). You should return a Vector<String> instead.

[Andrei Poenaru]

Created attachment 153473 [details]
Patch

[Early Warning System Bot]

Comment on attachment 153473 [details]
Patch

Attachment 153473 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13309224

[Early Warning System Bot]

Comment on attachment 153473 [details]
Patch

Attachment 153473 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13313174

[Andrei Poenaru]

Created attachment 153480 [details]
Patch

[WebKit Review Bot]

Comment on attachment 153480 [details]
Patch

Clearing flags on attachment: 153480

Committed r123205: <http://trac.webkit.org/changeset/123205>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.