Bug 66868 – Web Inspector: Option+Click on Node Expander Doesn't Work the First Time

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Joseph Pecoraro]

I think this is a regression. Option/Alt + Click on the node expando triangle
in the Elements tree hierarchy  does not expand subtrees the first time. Closing
and then reopening with alt+click the second and subsequent times works.

[Antoine Quint]

Created attachment 182131 [details]
Patch

[Antoine Quint]

*** Bug 106435 has been marked as a duplicate of this bug. ***

[Timothy Hatcher]

Comment on attachment 182131 [details]
Patch

numberOfArguments might not be enough in the future. We should check if an argument exists by name.

[Antoine Quint]

Created attachment 182142 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 182142 [details]
Patch

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

> Source/WebCore/inspector/front-end/DOMAgent.js:309
> +        var method = DOMAgent.requestChildNodes;
> +        if (method.numberOfArguments === 2)
> +            method(this.id, depth, mycallback.bind(this));
> +        else
> +            method(this.id, mycallback.bind(this))

Maybe one of the Chromium or Android developers can correct me on this but I don't think the OpenSource inspector does backwards compatibility protocol checking. You should just use DOMAgent.requestChildNodes expecting the current version. The OpenSource inspector here is bundled with WebKit and thus the generated InspectorBackendCommands file will always have the latest DOMAgent.requestChildNodes with the optional argument. Safari's inspector though needs backwards compatibility.

[Pavel Feldman]

Comment on attachment 182142 [details]
Patch

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

You also need a front-end test that is calling expand recursively.

Timothy: it would be great if you could give me a chance to make a review and point to the problems. I'll try to be responsive to not slow you down, but stamping everything without giving us a chance to look at it is not going to work well.

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

You should mention that the default is 1.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:483
> +    pushChildNodesToFrontend(nodeId, depth ? *depth : 1);

You should check for the depth being >= 0.

> Source/WebCore/inspector/front-end/DOMAgent.js:283
> +    getChildNodes: function(depth, callback)

I think you should leave this one as is and introduce getSubtree(depth) instead.

> Source/WebCore/inspector/front-end/DOMAgent.js:301
> +        // The requestChildNodes method has an optional depth parameter

This scenario is not necessary to support.

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1075
> +        TreeElement.prototype.expandRecursively.call(this, Number.MAX_VALUE);

I would do representedObject.getChildNodes(MAX_VALUE) and in the callback, I would call for the existing expandRecursively without any other modifications to the code.

> Source/WebCore/inspector/front-end/InspectorBackend.js:72
> +        window[agentName][domainAndMethod[1]]["numberOfArguments"] = signature.length || 0;

Why did this change?

> Source/WebCore/inspector/front-end/treeoutline.js:884
> +    // If the element has children, we want to make sure it's fully populated before

As mentioned above, I think populateWithDepth just makes things more complex. I wonder if you should simply go down the hierarchy and call expand instead? But before you call expandRecursively, you should fetch all the data from the backend.

> LayoutTests/inspector-protocol/dom-request-child-nodes-depth.html:13
> +    InspectorTest.eventHandler["DOM.setChildNodes"] = function (messageObject) {

{ should be on the next line, we don't use anonymous functions - should be a named one.

[Antoine Quint]

(In reply to comment #6)
> (From update of attachment 182142 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182142&action=review
> 
> You also need a front-end test that is calling expand recursively.

I'll look into providing one such test.

> > Source/WebCore/inspector/Inspector.json:1711
> > +                    { "name": "depth", "type": "integer", "optional": true, "description": "The maximum depth at which children should be retrieved." }
> 
> You should mention that the default is 1.

Will do.

> > Source/WebCore/inspector/InspectorDOMAgent.cpp:483
> > +    pushChildNodesToFrontend(nodeId, depth ? *depth : 1);
> 
> You should check for the depth being >= 0.

Will do.
 
> > Source/WebCore/inspector/front-end/DOMAgent.js:283
> > +    getChildNodes: function(depth, callback)
> 
> I think you should leave this one as is and introduce getSubtree(depth) instead.

Good point.

> > Source/WebCore/inspector/front-end/DOMAgent.js:301
> > +        // The requestChildNodes method has an optional depth parameter
> 
> This scenario is not necessary to support.

OK, thanks for the clarification. Joe was gathering as much. This will make the patch simpler.

> > Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1075
> > +        TreeElement.prototype.expandRecursively.call(this, Number.MAX_VALUE);
> 
> I would do representedObject.getChildNodes(MAX_VALUE) and in the callback, I would call for the existing expandRecursively without any other modifications to the code.

Interesting. I will try that.

> > Source/WebCore/inspector/front-end/InspectorBackend.js:72
> > +        window[agentName][domainAndMethod[1]]["numberOfArguments"] = signature.length || 0;
> 
> Why did this change?

This was for the backward compatibility issue which you mentioned was not necessary. I will remove this.

> > Source/WebCore/inspector/front-end/treeoutline.js:884
> > +    // If the element has children, we want to make sure it's fully populated before
> 
> As mentioned above, I think populateWithDepth just makes things more complex. I wonder if you should simply go down the hierarchy and call expand instead? But before you call expandRecursively, you should fetch all the data from the backend.

I will look into that.

> > LayoutTests/inspector-protocol/dom-request-child-nodes-depth.html:13
> > +    InspectorTest.eventHandler["DOM.setChildNodes"] = function (messageObject) {
> 
> { should be on the next line, we don't use anonymous functions - should be a named one.

Correct, thanks for pointing that out.

I will prepare an updated patch.

[Antoine Quint]

Created attachment 182217 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 182217 [details]
Patch

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

Looks good to me. I just want to negatives handled more directly.

> Source/WebCore/inspector/Inspector.json:1711
> +                    { "name": "depth", "type": "integer", "optional": true, "description": "The maximum depth at which children should be retrieved, defaults to 1." }

I still think you should document the range of this value. It sounds like 1 <= depth <= MAX_INT.

How does this work with Number.MAX_VALUE, a very large double, being truncated to an int? InspectorValues::asNumber(int*) just casts the m_doubleValue to an int, which seems like it could produce unexpected results:

    #include <stdio.h>
    int main() {
        double num = 1.7976931348623157e+308; // Number.MAX_VALUE
        printf("double: %e\nint: %d\n", num, (int)num);
    }

    // Produces:
    // double: 1.797693e+308
    // int: -2147483648

Obviously your tests show this was working though, so I wonder what I might be missing.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:483
> +    pushChildNodesToFrontend(nodeId, depth && depth >= 0 ? *depth : 1);

I still think it would be nice to produce a protocol error if *depth is <= 0 and to not support 0.

> Source/WebCore/inspector/front-end/DOMAgent.js:304
> +     * @param {int} depth

Nit (type): I believe this should be {number}, not {int}.

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1054
> +        var depth = Number.MAX_VALUE;

Nit: "var" -> "const" seems clearer here. (We don't really have a style guide for when to use const)

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1059
> +        };

Nit (style): we don't normally put a semicolon after inline function declarations

> LayoutTests/inspector/elements/expand-recursively-expected.txt:6
> +Running: initialExpand
> +
> +Running: deepExpand

You could dump at the initialExpand as well.

> LayoutTests/inspector/elements/expand-recursively.html:20
> +                for (var i = 0; children && i < children.length; ++i) {
> +                    children[i].expand();
> +                }

Nit (style): braces not needed for the single line for loop.

> LayoutTests/inspector/elements/expand-recursively.html:33
> +            setTimeout(next, 10);

Instead of this setTimeout I think you can use InspectorTest.runAfterPendingDispatches(next). The expandRecursively do your requestChildNodes, and runAfterPendingDispatches will wait until that message has its response dispatched.

[Pavel Feldman]

Comment on attachment 182217 [details]
Patch

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

This looks great, thanks for following up with the test. Few nits left and it is ready for landing.

>> Source/WebCore/inspector/Inspector.json:1711
>> +                    { "name": "depth", "type": "integer", "optional": true, "description": "The maximum depth at which children should be retrieved, defaults to 1." }
> 
> I still think you should document the range of this value. It sounds like 1 <= depth <= MAX_INT.
> 
> How does this work with Number.MAX_VALUE, a very large double, being truncated to an int? InspectorValues::asNumber(int*) just casts the m_doubleValue to an int, which seems like it could produce unexpected results:
> 
>     #include <stdio.h>
>     int main() {
>         double num = 1.7976931348623157e+308; // Number.MAX_VALUE
>         printf("double: %e\nint: %d\n", num, (int)num);
>     }
> 
>     // Produces:
>     // double: 1.797693e+308
>     // int: -2147483648
> 
> Obviously your tests show this was working though, so I wonder what I might be missing.

I wonder if we should instead allow -1 as a special case for unbounded. It is easier to explain.

>> Source/WebCore/inspector/InspectorDOMAgent.cpp:483
>> +    pushChildNodesToFrontend(nodeId, depth && depth >= 0 ? *depth : 1);
> 
> I still think it would be nice to produce a protocol error if *depth is <= 0 and to not support 0.

*errorString = "Please provide positive integer as a depth or -1 for entire subtree". Also, you should be comparing *depth, not depth to 0 (-1), otherwise you are checking the address value.

>> Source/WebCore/inspector/front-end/DOMAgent.js:304
>> +     * @param {int} depth
> 
> Nit (type): I believe this should be {number}, not {int}.

Yep. Putting closure's compiler.jar into ~/closure and running Source/WebCore/inspector/compile-front-end.py would check this and other issues. We don't require it formally, but all inspector contributors us the tool.

> Source/WebCore/inspector/front-end/DOMAgent.js:316
> +                callback(this.children);

You should run callback(null) otherwise. Call sites might have continuation they expect to run at all times.

> LayoutTests/inspector/elements/expand-recursively.html:11
> +    InspectorTest.runTestSuite([

It is not a test suite in case your steps are inter-dependent. In your case, it is simply a multi-step test. We just do a series of steps followed by an InspectorTest.completeTest in that case.

> LayoutTests/inspector/elements/expand-recursively.html:12
> +        function initialExpand(next)

Why do you need the initial expand? Does it expand first level? (html node?) In fact, elements panel will request document to the 3rd level on its own and expand the first two upon load.

> LayoutTests/inspector/elements/expand-recursively.html:18
> +                for (var i = 0; children && i < children.length; ++i) {

No need for {} around the single-line block.

> LayoutTests/inspector/elements/expand-recursively.html:24
> +            WebInspector.showPanel("elements");

This should be a first line in the test to load the panel code.

>> LayoutTests/inspector/elements/expand-recursively.html:33
>> +            setTimeout(next, 10);
> 
> Instead of this setTimeout I think you can use InspectorTest.runAfterPendingDispatches(next). The expandRecursively do your requestChildNodes, and runAfterPendingDispatches will wait until that message has its response dispatched.

We never do setTimeout(value), it is either setTimeout(0) or InspectorTest.runAfterPendingDispatches. The difference between the two is that setTimeout(0) is basically invokes your callback after current message loop entry. I.e. post your synchronous JS. InspectorTest.runAfterPendingDispatches is making sure all the pending protocol commands are processed and all the callbacks are received.

So in this case, Joe is absolutely right. Since expandRecursively is issueing the command, you need to use runAfterPendingDispatches. Timer won't help since on many platforms asynchronity of command processing is not timer-based. So timer may tick, but the request will still be up in the air. If your expandRecursively had a callback and you knew that the command you need is processed, your wouldn't even need the setTimeout since all onpopulate / getChildren would return synchronously.

Having an optional callback in expandRecursively or even better another method that would be used both by expandRecursively and this test, would make this test more robust. We try to not use runAfterPendingDispatches unless necessary.

[Antoine Quint]

Created attachment 182313 [details]
Patch

[Antoine Quint]

Created attachment 182314 [details]
Patch

[WebKit Review Bot]

Comment on attachment 182314 [details]
Patch

Clearing flags on attachment: 182314

Committed r139429: <http://trac.webkit.org/changeset/139429>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.