Bug 106435 – Web Inspector: add a new protocol method to InspectorDOMAgent to be able to request child nodes at a given depth

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Web Inspector (Deprecated)

[Antoine Quint]

In order to facilitate true deep expansion of nodes in the DOM tree (see https://bugs.webkit.org/show_bug.cgi?id=66868), we should be able to pass an extra parameter to InspectorDOMAgent::requestChildNodes(), and subsequently pushChildNodesToFrontend(), to specify the depth at which we want to gather children of a given node. This should be relatively easy since the method buildArrayForContainerChildren() already has support for specifying the depth and pushChildNodesToFrontend() relies on this method to gather the child nodes.

[Radar WebKit Bug Importer]

<rdar://problem/12980145>

[Antoine Quint]

While it would be simpler to just add an extra parameter to existing methods, the fact that in the DOM API a node's .childNodes are always the immediate descendant might prompt us to actually introduce other method with naming that might more appropriate. I'd like to hear people's suggestions on the topic.

[Joseph Pecoraro]

Ahh, so you think it would be clearer to have a function requestChildNodesWithDepth instead of requestChildNodes(depth). That seems fine to me. I don't have a preference.

[Antoine Quint]

Created attachment 181982 [details]
Patch

[Joseph Pecoraro]

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

> Source/WebCore/inspector/InspectorDOMAgent.cpp:488
> +    pushChildNodesToFrontend(nodeId, depth);

You should produce an error or handle the case of depth <= 0. Negative doesn't make sense, and it looks like 0 is a special case internally to only get the text node children. Also document this in the protocol file. An existing example would be setDeviceMetricsOverride. It documents its range in Inspector.json and produces an error for negative values.

You could also add an ASSERT in buildArrayForContainerChildren that depth is >= 0.

[Joseph Pecoraro]

Comment on attachment 181982 [details]
Patch

r=me with handling <= 0

[Joseph Pecoraro]

You may also want to update the title of the bug + thus ChangeLog. Since now you're not adding a param, you're adding a new function.

[Joseph Pecoraro]

Comment on attachment 181982 [details]
Patch

Actually, this probably requires at least a basic test. r- for an updated patch containing a test.

[Pavel Feldman]

Comment on attachment 181982 [details]
Patch

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

You need a test and a corresponding front-end fix that would expand the entire branch. Exposing capabilities / fixing bugs for proprietary front-ends and not fixing it upstream is a no go.

> Source/WebCore/inspector/Inspector.json:1715
> +                "name": "requestChildNodesWithDepth",

You should not add public methods unless you are committing to supporting them.

> Source/WebCore/inspector/Inspector.json:1718
> +                    { "name": "depth", "type": "integer", "description": "The maximum depth at which children should be retrieved." }

This should be an optional parameter of the requestChildNodes above.

[Antoine Quint]

(In reply to comment #9)
> (From update of attachment 181982 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181982&action=review
> 
> You need a test and a corresponding front-end fix that would expand the entire branch. Exposing capabilities / fixing bugs for proprietary front-ends and not fixing it upstream is a no go.
> 
> > Source/WebCore/inspector/Inspector.json:1715
> > +                "name": "requestChildNodesWithDepth",
> 
> You should not add public methods unless you are committing to supporting them.
> 
> > Source/WebCore/inspector/Inspector.json:1718
> > +                    { "name": "depth", "type": "integer", "description": "The maximum depth at which children should be retrieved." }
> 
> This should be an optional parameter of the requestChildNodes above.

Yes, that's what I'm going to do. I may close this bug as a DUPLICATE and fix https://bugs.webkit.org/show_bug.cgi?id=66868 instead.

[Antoine Quint]

Making the discussed backend enhancement as well as front-end fixes in https://bugs.webkit.org/show_bug.cgi?id=66868.

*** This bug has been marked as a duplicate of bug 66868 ***

[Joseph Pecoraro]

(In reply to comment #9)
> (From update of attachment 181982 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181982&action=review
> 
> > Source/WebCore/inspector/Inspector.json:1718
> > +                    { "name": "depth", "type": "integer", "description": "The maximum depth at which children should be retrieved." }
> 
> This should be an optional parameter of the requestChildNodes above.

Yes, Antoine and I had an IRC discussion about this after the review and we came to the same conclusion. I forgot to comment here.