Bug 92739 - Web Inspector: Protocol: Add "namedFlowCreated" and "namedFlowRemoved" events
Summary: Web Inspector: Protocol: Add "namedFlowCreated" and "namedFlowRemoved" events
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on: 93082
Blocks: 90786
  Show dependency treegraph
Reported: 2012-07-31 04:13 PDT by Andrei Poenaru
Modified: 2012-08-06 09:40 PDT (History)
11 users (show)

See Also:

Inspector Extension (3.03 KB, application/octet-stream)
2012-07-31 04:15 PDT, Andrei Poenaru
no flags Details
Patch (17.75 KB, patch)
2012-07-31 06:53 PDT, Andrei Poenaru
pfeldman: review-
Details | Formatted Diff | Diff
Patch (18.43 KB, patch)
2012-08-03 01:21 PDT, Andrei Poenaru
pfeldman: review-
Details | Formatted Diff | Diff
Patch (18.26 KB, patch)
2012-08-03 04:20 PDT, Andrei Poenaru
pfeldman: review-
Details | Formatted Diff | Diff
Patch (18.79 KB, patch)
2012-08-06 07:58 PDT, Andrei Poenaru
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrei Poenaru 2012-07-31 04:13:59 PDT
Extends the protocol with "namedFlowCreated" and "namedFlowRemoved" events.
Comment 1 Andrei Poenaru 2012-07-31 04:15:02 PDT
Created attachment 155486 [details]
Inspector Extension
Comment 2 Andrei Poenaru 2012-07-31 06:53:15 PDT
Created attachment 155521 [details]
Comment 3 Pavel Feldman 2012-08-02 12:00:32 PDT
Comment on attachment 155521 [details]

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

> Source/WebCore/inspector/InspectorCSSAgent.cpp:540
> +    int nodeId = m_domAgent->boundNodeId(static_cast<Node*>(document));

There is no need to cast document to Node, Document is a Node.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:541
> +    if (!nodeId)

I am not sure this is what you want. You only report creation / destruction of the flows when document node on the front-end is visible. So every time document node is received on the front-end (user expanded an <iframe> tag), you want to query for document's existing named flows and dispatch corresponding events.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:550
> +    int nodeId = m_domAgent->boundNodeId(static_cast<Node*>(document));

Comment 4 Andrei Poenaru 2012-08-03 01:21:02 PDT
Created attachment 156285 [details]
Comment 5 Pavel Feldman 2012-08-03 02:31:56 PDT
Comment on attachment 156285 [details]

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

Couple more nits, one of them depends on another bug Alexander is filing.

> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:83

You should not surround Instrumentation calls with #if ENABLE.

> Source/WebCore/dom/WebKitNamedFlowCollection.cpp:101


> Source/WebCore/inspector/InspectorCSSAgent.cpp:540
> +    int nodeId = m_domAgent->pushNodePathToFrontend(document);

pushChildNodesToFrontend implies there is a front-end.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:542
> +    if (m_frontend)

You should not make this check. In fact, you should not be getting into this method when there is no m_frontend. It is a bug in InspectorCSSAgent that needs to be addressed. I'll ask apavlov@ to handle it.
Comment 6 Pavel Feldman 2012-08-03 02:32:08 PDT
Comment on attachment 156285 [details]

r- per comments
Comment 7 Andrei Poenaru 2012-08-03 04:20:46 PDT
Created attachment 156323 [details]
Comment 8 Pavel Feldman 2012-08-06 06:34:38 PDT
Comment on attachment 156323 [details]

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

Looks good, once issue below...

> Source/WebCore/inspector/InspectorCSSAgent.cpp:540
> +    m_frontend->namedFlowCreated(m_domAgent->pushNodePathToFrontend(document), name.string());

Sorry for not spotting this earlier. By design, protocol should not emit any events unless they are explicitly requested by the client. In your case, attaching to the backend will start populating DOM with nodes and issuing these events. So we need to figure out how to prevent that from happening. What we try to do in such cases is:
- events are not generated unless getNamedFlowCollection is called for given document node
- once getNamedFlowCollection was called at least once, namedFromCreated/Deleted events start flowing to the front-end

That will make behaviour consistent: once you asked for the present state, you get update notifications, you would not need to call pushNodePathToFrontend anymore - boundNodeId would be sufficient.
Comment 9 Andrei Poenaru 2012-08-06 07:58:50 PDT
Created attachment 156695 [details]
Comment 10 WebKit Review Bot 2012-08-06 09:40:12 PDT
Comment on attachment 156695 [details]

Clearing flags on attachment: 156695

Committed r124778: <http://trac.webkit.org/changeset/124778>
Comment 11 WebKit Review Bot 2012-08-06 09:40:16 PDT
All reviewed patches have been landed.  Closing bug.