[devtools] Return empty array from DOMNode.children() rather than null.

[devtools] Return empty array from DOMNode.children() rather than null.

Array.<WebInspector.DOMNode> for no children should be [].

See comments on https://codereview.chromium.org/16561003/

BUG=

[johnjbarton, 2013-10-08 22:00:41]

ready for review

[pfeldman, 2013-10-09 05:41:05]

https://codereview.chromium.org/26586002/diff/1/Source/devtools/front_end/DOM...
File Source/devtools/front_end/DOMAgent.js (right):

https://codereview.chromium.org/26586002/diff/1/Source/devtools/front_end/DOM...
Source/devtools/front_end/DOMAgent.js:141: return this._children ?
this._children.slice() : [];
This is intentionally null. We should instead do:

children: function()
{
    if (!this._children)
        throw new Error("Requesting children prior to getting them from
backend");
    return this._children;
}

Client should only use this method when they are confident node has already
received children asynchronously and they can proceed with synchronous
operation. This applies to ElementsTreeOutline call sites. If that is not the
case, please file a bug.

[johnjbarton, 2013-10-09 17:12:55]

On 2013/10/09 05:41:05, pfeldman wrote:
>
https://codereview.chromium.org/26586002/diff/1/Source/devtools/front_end/DOM...
> File Source/devtools/front_end/DOMAgent.js (right):
> 
>
https://codereview.chromium.org/26586002/diff/1/Source/devtools/front_end/DOM...
> Source/devtools/front_end/DOMAgent.js:141: return this._children ?
> this._children.slice() : [];
> This is intentionally null. We should instead do:
> 
> children: function()
> {
>     if (!this._children)
>         throw new Error("Requesting children prior to getting them from
> backend");
>     return this._children;
> }
> 
> Client should only use this method when they are confident node has already
> received children asynchronously and they can proceed with synchronous
> operation. This applies to ElementsTreeOutline call sites. If that is not the
> case, please file a bug.


Well when I apply that change 10 tests fail with the thrown Error.

 test 	results		actual failure	expected failure
+http/tests/inspector/inspect-iframe-from-different-domain.html	expected actual
diff pretty diff wdiff		text	pass
+http/tests/inspector/modify-cross-domain-rule.html	expected actual diff pretty
diff wdiff		text	pass
+http/tests/inspector/styles/edit-css-with-source-url.html	expected actual diff
pretty diff wdiff		text	pass
+http/tests/inspector/styles/selector-line-deprecated.html	expected actual diff
pretty diff wdiff		text	pass
+http/tests/inspector/styles/selector-line-sourcemap-header-deprecated.html	expected
actual diff pretty diff wdiff		text	pass
+http/tests/inspector/styles/selector-line-sourcemap-header.html	expected actual
diff pretty diff wdiff		text	pass
+http/tests/inspector/styles/selector-line.html	expected actual diff pretty diff
wdiff		text	pass
+http/tests/inspector/styles/styles-redirected-css.html	expected actual diff
pretty diff wdiff		text	pass
+http/tests/inspector/styles/stylesheet-tracking.html	expected actual diff
pretty diff wdiff		text	pass
+http/tests/inspector/styles/xsl-transformed.xml	expected actual diff pretty
diff wdiff		text	pass

[pfeldman, 2013-10-09 18:13:04]

I'll take a look tomorrow my time.

[pfeldman, 2013-10-10 15:02:37]

Could you provide bug with the repro steps? There is a test for the scenario
that you claim fails and it passes...

[johnjbarton, 2013-10-10 15:14:03]

On 2013/10/10 15:02:37, pfeldman wrote:
> Could you provide bug with the repro steps? There is a test for the scenario
> that you claim fails and it passes...

I'm confused by your question.

When I change the code per your suggestion, 10 layout tests fail. So it appears
that DOMNode.children is called before the children arrive from async calls in
these 10 cases: there must be a code path that fails your design assumption. 
These 10 tests pass if we return null from DOMNode.children(), suggesting that
the result is not used in the tests which is why they pass normally.

Are you asking for a test case showing the exception based on returning null
(current code)?  I hit this when debugging devtools with pause-on-exceptions.

[pfeldman, 2013-10-10 15:20:46]

On 2013/10/10 15:14:03, johnjbarton wrote:
> On 2013/10/10 15:02:37, pfeldman wrote:
> > Could you provide bug with the repro steps? There is a test for the scenario
> > that you claim fails and it passes...
> 
> I'm confused by your question.
> 

Sorry, my question was a request for the repro steps your change addresses. At
first I was mistakenly assuming it was about the scenario in
https://codereview.chromium.org/16561003/ which had a test that passes...

> When I change the code per your suggestion, 10 layout tests fail. So it
appears
> that DOMNode.children is called before the children arrive from async calls in
> these 10 cases: there must be a code path that fails your design assumption. 

Not really, that is DOMBreakpointsSidebarPane that has the following code:

        var children = node.children();
        if (!children)
            return;

So we should not throw exception there, but clients should expect null result. I
uploaded my vision of the provisional fix, but I can't verify it since there are
no repro steps.

> These 10 tests pass if we return null from DOMNode.children(), suggesting that
> the result is not used in the tests which is why they pass normally.
> 
> Are you asking for a test case showing the exception based on returning null
> (current code)?  I hit this when debugging devtools with pause-on-exceptions.

I'm rather interested if patch #2 fixes it for you and if yes, I'd be happy to
write a test.

[johnjbarton, 2013-10-10 22:31:31]

On 2013/10/10 15:20:46, pfeldman wrote:
> On 2013/10/10 15:14:03, johnjbarton wrote:
> > On 2013/10/10 15:02:37, pfeldman wrote:
> > > Could you provide bug with the repro steps? There is a test for the
scenario
> > > that you claim fails and it passes...
> > 
> > I'm confused by your question.
> > 
> 
> Sorry, my question was a request for the repro steps your change addresses. At
> first I was mistakenly assuming it was about the scenario in
> https://codereview.chromium.org/16561003/ which had a test that passes...
> 
> > When I change the code per your suggestion, 10 layout tests fail. So it
> appears
> > that DOMNode.children is called before the children arrive from async calls
in
> > these 10 cases: there must be a code path that fails your design assumption.

> 
> Not really, that is DOMBreakpointsSidebarPane that has the following code:
> 
>         var children = node.children();
>         if (!children)
>             return;
> 
> So we should not throw exception there, but clients should expect null result.
I
> uploaded my vision of the provisional fix, but I can't verify it since there
are
> no repro steps.
> 
> > These 10 tests pass if we return null from DOMNode.children(), suggesting
that
> > the result is not used in the tests which is why they pass normally.
> > 
> > Are you asking for a test case showing the exception based on returning null
> > (current code)?  I hit this when debugging devtools with
pause-on-exceptions.
> 
> I'm rather interested if patch #2 fixes it for you and if yes, I'd be happy to
> write a test.

I didn't save the app that failed unfortunately. I'll keep looking for it.

[pfeldman, 2013-10-17 17:20:12]

Should we close it for now?