DevTools: Inspect element mode does not select element in ShadowDOM

DevTools: Inspect element mode does not select element in ShadowDOM

BUG=236509
R=vsevik@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152016

[apavlov, 2013-06-06 16:18:59]

Looks safe to me.

https://codereview.chromium.org/16561003/diff/1/LayoutTests/http/tests/inspec...
File LayoutTests/http/tests/inspector/elements-test.js (right):

https://codereview.chromium.org/16561003/diff/1/LayoutTests/http/tests/inspec...
LayoutTests/http/tests/inspector/elements-test.js:26: } finally {
Curlies not necessary here? The new styleguide is a bit ambiguous.

https://codereview.chromium.org/16561003/diff/1/LayoutTests/http/tests/inspec...
LayoutTests/http/tests/inspector/elements-test.js:30: if (!result &&
!pendingRequests) {
ditto

https://codereview.chromium.org/16561003/diff/1/Source/devtools/front_end/Ele...
File Source/devtools/front_end/ElementsTreeOutline.js (right):

https://codereview.chromium.org/16561003/diff/1/Source/devtools/front_end/Ele...
Source/devtools/front_end/ElementsTreeOutline.js:883: if (index == -1)
===

https://codereview.chromium.org/16561003/diff/1/Source/devtools/front_end/Ele...
Source/devtools/front_end/ElementsTreeOutline.js:2114: visibleChildren =
visibleChildren.concat(this._node.children());
Alternatively, visibleChildren.push.apply(visibleChildren,
this._node.children());
which saves a new array allocation/copy.

[vsevik, 2013-06-07 08:40:59]

lgtm

https://chromiumcodereview.appspot.com/16561003/diff/1/LayoutTests/http/tests...
File LayoutTests/http/tests/inspector/elements-test.js (right):

https://chromiumcodereview.appspot.com/16561003/diff/1/LayoutTests/http/tests...
LayoutTests/http/tests/inspector/elements-test.js:5: var originalCallback =
callback;
Unused.

https://chromiumcodereview.appspot.com/16561003/diff/1/LayoutTests/http/tests...
LayoutTests/http/tests/inspector/elements-test.js:20: pendingRequests--;
remove

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/core/inspector/...
File Source/core/inspector/InspectorDOMAgent.cpp (right):

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/core/inspector/...
Source/core/inspector/InspectorDOMAgent.cpp:1425: if (children->length() > 0 ||
forcePushChildren) // Push children along with shadow in any case.
|| depth

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/devtools/front_...
File Source/devtools/front_end/DOMAgent.js (right):

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/devtools/front_...
Source/devtools/front_end/DOMAgent.js:504: if (!this._children) {
Impossible, thanks to my || depth check.

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/devtools/front_...
Source/devtools/front_end/DOMAgent.js:505: this._children = [];
 = [node]

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/devtools/front_...
Source/devtools/front_end/DOMAgent.js:508: this._children.unshift(node);
probably not needed

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/devtools/front_...
File Source/devtools/front_end/ElementsTreeOutline.js (left):

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/devtools/front_...
Source/devtools/front_end/ElementsTreeOutline.js:277: if (treeElement &&
treeElement.showChild(node.index))
.index is unused now

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/devtools/front_...
Source/devtools/front_end/ElementsTreeOutline.js:1005: var child =
node.firstChild;
unused now

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/devtools/front_...
Source/devtools/front_end/ElementsTreeOutline.js:1033: child =
child.nextSibling;
unused now

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/devtools/front_...
File Source/devtools/front_end/ElementsTreeOutline.js (right):

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/devtools/front_...
Source/devtools/front_end/ElementsTreeOutline.js:2027: var newNode = parentNode
? parentNode.children()[index] || parentNode : null;
...Or index is used.....???

[pfeldman, 2013-06-07 08:53:48]

https://chromiumcodereview.appspot.com/16561003/diff/1/LayoutTests/http/tests...
File LayoutTests/http/tests/inspector/elements-test.js (right):

https://chromiumcodereview.appspot.com/16561003/diff/1/LayoutTests/http/tests...
LayoutTests/http/tests/inspector/elements-test.js:5: var originalCallback =
callback;
On 2013/06/07 08:41:00, vsevik wrote:
> Unused.

Done.

https://chromiumcodereview.appspot.com/16561003/diff/1/LayoutTests/http/tests...
LayoutTests/http/tests/inspector/elements-test.js:20: pendingRequests--;
On 2013/06/07 08:41:00, vsevik wrote:
> remove

Done.

https://chromiumcodereview.appspot.com/16561003/diff/1/LayoutTests/http/tests...
LayoutTests/http/tests/inspector/elements-test.js:30: if (!result &&
!pendingRequests) {
On 2013/06/06 16:18:59, apavlov wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/core/inspector/...
File Source/core/inspector/InspectorDOMAgent.cpp (right):

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/core/inspector/...
Source/core/inspector/InspectorDOMAgent.cpp:1425: if (children->length() > 0 ||
forcePushChildren) // Push children along with shadow in any case.
On 2013/06/07 08:41:00, vsevik wrote:
> || depth

Done.

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/devtools/front_...
File Source/devtools/front_end/DOMAgent.js (right):

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/devtools/front_...
Source/devtools/front_end/DOMAgent.js:504: if (!this._children) {
On 2013/06/07 08:41:00, vsevik wrote:
> Impossible, thanks to my || depth check.

Done.

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/devtools/front_...
Source/devtools/front_end/DOMAgent.js:508: this._children.unshift(node);
On 2013/06/07 08:41:00, vsevik wrote:
> probably not needed

Done.

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/devtools/front_...
File Source/devtools/front_end/ElementsTreeOutline.js (left):

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/devtools/front_...
Source/devtools/front_end/ElementsTreeOutline.js:1005: var child =
node.firstChild;
Still is, sorry

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/devtools/front_...
File Source/devtools/front_end/ElementsTreeOutline.js (right):

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/devtools/front_...
Source/devtools/front_end/ElementsTreeOutline.js:883: if (index == -1)
On 2013/06/06 16:18:59, apavlov wrote:
> ===

Done.

https://chromiumcodereview.appspot.com/16561003/diff/1/Source/devtools/front_...
Source/devtools/front_end/ElementsTreeOutline.js:2114: visibleChildren =
visibleChildren.concat(this._node.children());
On 2013/06/06 16:18:59, apavlov wrote:
> Alternatively, visibleChildren.push.apply(visibleChildren,
> this._node.children());
> which saves a new array allocation/copy.

Done.

[commit-bot: I haz the power, 2013-06-07 09:41:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pfeldman@chromium.org/16561003/10001

[commit-bot: I haz the power, 2013-06-07 10:36:30]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[pfeldman, 2013-06-07 11:44:00]

Committed patchset #2 manually as r152016 (presubmit successful).

[johnjbarton, 2013-10-08 21:59:56]

See https://codereview.chromium.org/26586002

https://codereview.chromium.org/16561003/diff/10001/Source/devtools/front_end...
File Source/devtools/front_end/DOMAgent.js (right):

https://codereview.chromium.org/16561003/diff/10001/Source/devtools/front_end...
Source/devtools/front_end/DOMAgent.js:133: return this._children ?
this._children.slice() : null;
I don't know why this has not caused problems before, but the null return here
is not an array, so when we all concat we get an array 
with a null:

> [].concat(this._children ? this._children.slice() : null)
[null]

https://codereview.chromium.org/16561003/diff/10001/Source/devtools/front_end...
File Source/devtools/front_end/ElementsTreeOutline.js (right):

https://codereview.chromium.org/16561003/diff/10001/Source/devtools/front_end...
Source/devtools/front_end/ElementsTreeOutline.js:1979: if
(this._node.templateContent() || (WebInspector.settings.showShadowDOM.get() &&
this._node.hasShadowRoots()))
Here we throw an exception because this._node is null, after we descend in to
children on a children array [null].

https://codereview.chromium.org/16561003/diff/10001/Source/devtools/front_end...
Source/devtools/front_end/ElementsTreeOutline.js:2115: visibleChildren =
visibleChildren.concat(this._node.children());
Here is the concat which uses childern(). visibleChildren becomes [null].