[webkit-reviews] review denied: [Bug 66868] Web Inspector: Option+Click on Node Expander Doesn't Work the First Time : [Attachment 182217] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Antoine Quint
<graouts at apple.com>'s request for review:
Bug 66868: Web Inspector: Option+Click on Node Expander Doesn't Work the First
Time
https://bugs.webkit.org/show_bug.cgi?id=66868

Attachment 182217: Patch
https://bugs.webkit.org/attachment.cgi?id=182217&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.