DevTools: better hasSelection check to include composed trees

DevTools: better hasSelection check to include composed trees

When shadow elements receive an event, the handler may to check for selections
with hasSelection(). This CL makes hasSelection additionally check any composed
subtrees by recursively calling itself on distributed nodes.

BUG=674739
Committed: https://crrev.com/f9da6579a97d1e0be87de603df160a6c9358c0ae
Cr-Commit-Position: refs/heads/master@{#440332}

[luoe, 2016-12-19 22:51:53]

Description was changed from

==========
DevTools: better hasSelection check to include composed trees

BUG=674739
==========

to

==========
DevTools: better hasSelection check to include composed trees

When shadow elements receive an event, the handler may to check for selections
with hasSelection(). This CL makes hasSelection additionally check any composed
subtrees by recursively calling itself on distributed nodes.

BUG=674739
==========

[luoe, 2016-12-19 22:56:04]

luoe@chromium.org changed reviewers:
+ allada@chromium.org, lushnikov@chromium.org

[luoe, 2016-12-19 22:56:05]

ptal

[allada, 2016-12-20 02:15:29]

https://codereview.chromium.org/2588833004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/platform/DOMExtension.js
(right):

https://codereview.chromium.org/2588833004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/platform/DOMExtension.js:283: var
contentElement = contents[i].getDistributedNodes();
Wouldnt it be easier to read using something like:
for (var content of contents) {
  if (content.getDistributedNodes().some(element => element.hasSelection()))
    return true;
}

Or even do it all in 1 line using two Array.some() callbacks if it does't over
complicate it.

[luoe, 2016-12-20 02:41:03]

https://codereview.chromium.org/2588833004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/platform/DOMExtension.js
(right):

https://codereview.chromium.org/2588833004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/platform/DOMExtension.js:283: var
contentElement = contents[i].getDistributedNodes();
On 2016/12/20 02:15:28, allada wrote:
> Wouldnt it be easier to read using something like:
> for (var content of contents) {
>   if (content.getDistributedNodes().some(element => element.hasSelection()))
>     return true;
> }
> 
> Or even do it all in 1 line using two Array.some() callbacks if it does't over
> complicate it.

Done.

[allada, 2016-12-20 18:41:36]

https://codereview.chromium.org/2588833004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/platform/DOMExtension.js
(right):

https://codereview.chromium.org/2588833004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/platform/DOMExtension.js:283: if
(Array.prototype.some.call(content.getDistributedNodes(), node =>
node.hasSelection()))
Why Array.prototype.some.call?

Why not just access it with:
content.getDistributedNodes().some(node => node.hasSelection())

?

[luoe, 2016-12-20 19:23:58]

https://codereview.chromium.org/2588833004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/platform/DOMExtension.js
(right):

https://codereview.chromium.org/2588833004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/platform/DOMExtension.js:283: if
(Array.prototype.some.call(content.getDistributedNodes(), node =>
node.hasSelection()))
getDistributedNodes() returns a NodeList, which doesn't have some()
unfortunately.

Alternatively, it could be a forEach that sets hasSelection = hasSelection ||
node.hasSelection(), but I think the current way may be easier to read.

[allada, 2016-12-21 23:28:26]

lgtm

https://codereview.chromium.org/2588833004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/platform/DOMExtension.js
(right):

https://codereview.chromium.org/2588833004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/platform/DOMExtension.js:283: if
(Array.prototype.some.call(content.getDistributedNodes(), node =>
node.hasSelection()))
On 2016/12/20 19:23:58, luoe wrote:
> getDistributedNodes() returns a NodeList, which doesn't have some()
> unfortunately.
> 
> Alternatively, it could be a forEach that sets hasSelection = hasSelection ||
> node.hasSelection(), but I think the current way may be easier to read.

I see.

[luoe, 2016-12-21 23:35:34]

The CQ bit was checked by luoe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 23:35:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 01:09:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 01:09:10]

Dry run: This issue passed the CQ dry run.

[luoe, 2016-12-22 01:11:04]

The CQ bit was checked by luoe@chromium.org

[commit-bot: I haz the power, 2016-12-22 01:11:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 01:19:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 01:19:42]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[luoe, 2016-12-22 01:27:43]

luoe@chromium.org changed reviewers:
+ dgozman@chromium.org

[luoe, 2016-12-22 01:27:43]

Please take a look.  Although there is a lgtm, this CL requires one from an
owner.

[dgozman, 2016-12-22 01:59:33]

lgtm

https://codereview.chromium.org/2588833004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/platform/DOMExtension.js
(right):

https://codereview.chromium.org/2588833004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/platform/DOMExtension.js:280: //
FIXME: use contains(node, {includeShadow: true}) when it is fixed for shadow
dom.
TODO(luoe)

[luoe, 2016-12-22 02:03:56]

Thanks for the reviews!

https://codereview.chromium.org/2588833004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/platform/DOMExtension.js
(right):

https://codereview.chromium.org/2588833004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/platform/DOMExtension.js:280: //
FIXME: use contains(node, {includeShadow: true}) when it is fixed for shadow
dom.
On 2016/12/22 01:59:33, dgozman wrote:
> TODO(luoe)

Done.

[luoe, 2016-12-22 02:04:01]

The CQ bit was checked by luoe@chromium.org

[luoe, 2016-12-22 02:04:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, allada@chromium.org
Link to the patchset: https://codereview.chromium.org/2588833004/#ps60001
(title: "ac")

[commit-bot: I haz the power, 2016-12-22 02:04:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 03:50:30]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482372241264810,
"parent_rev": "3738ab8f56352a39fbc1e9b24e15d09b5e40be6b", "commit_rev":
"a7916885f13b3d9336f3195e970117e5d7622b82"}

[commit-bot: I haz the power, 2016-12-22 03:51:01]

Description was changed from

==========
DevTools: better hasSelection check to include composed trees

When shadow elements receive an event, the handler may to check for selections
with hasSelection(). This CL makes hasSelection additionally check any composed
subtrees by recursively calling itself on distributed nodes.

BUG=674739
==========

to

==========
DevTools: better hasSelection check to include composed trees

When shadow elements receive an event, the handler may to check for selections
with hasSelection(). This CL makes hasSelection additionally check any composed
subtrees by recursively calling itself on distributed nodes.

BUG=674739

Review-Url: https://codereview.chromium.org/2588833004
==========

[commit-bot: I haz the power, 2016-12-22 03:51:02]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-22 03:53:52]

Description was changed from

==========
DevTools: better hasSelection check to include composed trees

When shadow elements receive an event, the handler may to check for selections
with hasSelection(). This CL makes hasSelection additionally check any composed
subtrees by recursively calling itself on distributed nodes.

BUG=674739

Review-Url: https://codereview.chromium.org/2588833004
==========

to

==========
DevTools: better hasSelection check to include composed trees

When shadow elements receive an event, the handler may to check for selections
with hasSelection(). This CL makes hasSelection additionally check any composed
subtrees by recursively calling itself on distributed nodes.

BUG=674739

Committed: https://crrev.com/f9da6579a97d1e0be87de603df160a6c9358c0ae
Cr-Commit-Position: refs/heads/master@{#440332}
==========

[commit-bot: I haz the power, 2016-12-22 03:53:53]

Patchset 4 (id:??) landed as
https://crrev.com/f9da6579a97d1e0be87de603df160a6c9358c0ae
Cr-Commit-Position: refs/heads/master@{#440332}