Implement Node.isConnected

Implement Node.isConnected

This CL implements a Node atrribute Node.isConnected. Node.isConnected returns true when a context object is in a shadow-including document, and false otherwise.
See the spec here: https://dom.spec.whatwg.org/#dom-node-isconnected
See the discussion here: https://github.com/w3c/webcomponents/issues/81

BUG=599743
Committed: https://crrev.com/b05e567bbdffc0bd099ab835cdbcbded909550da
Cr-Commit-Position: refs/heads/master@{#385990}

[yuzuchan, 2016-04-04 04:55:46]

yuzus@chromium.org changed reviewers:
+ hayato@chromium.org, kochi@chromium.org

[yuzuchan, 2016-04-04 04:55:46]

PTAL

This is how I feel about this patch: http://i.imgur.com/0X6VSve.gif

[hayato, 2016-04-04 05:32:53]

Could you send an "Intent to implement and ship: Node.isConnected" in blink-dev?

https://codereview.chromium.org/1855823003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/1855823003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Node.cpp:786: bool Node::isConnected() const
I think we can use Node::inDocument() for Node.isConnected.

However, could you rename Node::inDocument() to Node::nShadowIncludingDocument()
in another CL?

See https://dom.spec.whatwg.org/#in-a-shadow-including-document

Unfortunately, the current Node::inDocument does mean "in a shadow including
document", I think.
It does not mean "In a document" in the spec.
https://dom.spec.whatwg.org/#in-a-document

That's confusing.

[yuzuchan, 2016-04-04 07:09:30]

This is how I feel about this patch: http://i.imgur.com/gPHIlxW.gif

https://codereview.chromium.org/1855823003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/1855823003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Node.cpp:786: bool Node::isConnected() const
On 2016/04/04 at 05:32:52, hayato wrote:
> I think we can use Node::inDocument() for Node.isConnected.
> 
> However, could you rename Node::inDocument() to
Node::nShadowIncludingDocument() in another CL?
> 
> See https://dom.spec.whatwg.org/#in-a-shadow-including-document
> 
> Unfortunately, the current Node::inDocument does mean "in a shadow including
document", I think.
> It does not mean "In a document" in the spec.
https://dom.spec.whatwg.org/#in-a-document
> 
> That's confusing.

Acknowledged.
I will start another CL to rename inDocument soon. :)

[kochi, 2016-04-04 09:22:43]

https://codereview.chromium.org/1855823003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html (right):

https://codereview.chromium.org/1855823003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:5: <script
src='resources/shadow-dom.js'></script>
Can you remove line 4-5?
I don't see any function in these js lib is used in this test.

https://codereview.chromium.org/1855823003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:23: 
nit: unnecessary newline?

I think it's okay to merge above 2 tests into one test().

https://codereview.chromium.org/1855823003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:35:
assert_false(orphan3.isConnected);
It's better to check all the ancestors (host2.shadowRoot, host2) is not
connected here.

https://codereview.chromium.org/1855823003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:37: 
Could you also add a test case where shadow host is appended in a shadow tree
(and then parenting shadow host is detached from the document)?

[yuzuchan, 2016-04-05 04:25:23]

Thanks for the review :) 

This is how I feel about this patch: http://i.imgur.com/NOw4wiQ.gif

https://codereview.chromium.org/1855823003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html (right):

https://codereview.chromium.org/1855823003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:5: <script
src='resources/shadow-dom.js'></script>
On 2016/04/04 at 09:22:42, kochi wrote:
> Can you remove line 4-5?
> I don't see any function in these js lib is used in this test.

Done.

https://codereview.chromium.org/1855823003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:23: 
On 2016/04/04 at 09:22:42, kochi wrote:
> nit: unnecessary newline?
> 
> I think it's okay to merge above 2 tests into one test().

Done.

https://codereview.chromium.org/1855823003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:35:
assert_false(orphan3.isConnected);
On 2016/04/04 at 09:22:42, kochi wrote:
> It's better to check all the ancestors (host2.shadowRoot, host2) is not
connected here.

Done.

https://codereview.chromium.org/1855823003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:37: 
On 2016/04/04 at 09:22:42, kochi wrote:
> Could you also add a test case where shadow host is appended in a shadow tree
> (and then parenting shadow host is detached from the document)?

Done.

[kochi, 2016-04-05 05:32:28]

https://codereview.chromium.org/1855823003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html (right):

https://codereview.chromium.org/1855823003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:10: var orphan1
= document.createElement('div');
Now you can move these 2 lines into the test function below.

https://codereview.chromium.org/1855823003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:18:
test(function() {
Probably this test should come first, before the test above.

https://codereview.chromium.org/1855823003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:19: var orphan2
= document.createElement('div');
How about adding
assert_false(orphan2.isConnected)
here?

https://codereview.chromium.org/1855823003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:46:
assert_false(orphan4.isConnected);
How about adding
host.appendChild(host3);
here and check all the descendants of host3's isConnected
turn true?

[yuzuchan, 2016-04-05 06:48:48]

Thanks for the review again! :) 

This is how I feel about this patch: http://i.imgur.com/BqTs1ZC.gif

https://codereview.chromium.org/1855823003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html (right):

https://codereview.chromium.org/1855823003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:10: var orphan1
= document.createElement('div');
On 2016/04/05 at 05:32:28, kochi wrote:
> Now you can move these 2 lines into the test function below.

Done.

https://codereview.chromium.org/1855823003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:18:
test(function() {
On 2016/04/05 at 05:32:28, kochi wrote:
> Probably this test should come first, before the test above.

Done.

https://codereview.chromium.org/1855823003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:19: var orphan2
= document.createElement('div');
On 2016/04/05 at 05:32:28, kochi wrote:
> How about adding
> assert_false(orphan2.isConnected)
> here?

Done.

https://codereview.chromium.org/1855823003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:46:
assert_false(orphan4.isConnected);
On 2016/04/05 at 05:32:28, kochi wrote:
> How about adding
> host.appendChild(host3);
> here and check all the descendants of host3's isConnected
> turn true?

Done.

[kochi, 2016-04-05 07:06:38]

https://codereview.chromium.org/1855823003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html (right):

https://codereview.chromium.org/1855823003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:15: }, 'When
inserted into a document tree, isConnected should be true.');
Could you update the description string for all tests in this file
to match the changes?

[kochi, 2016-04-05 07:06:39]

https://codereview.chromium.org/1855823003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html (right):

https://codereview.chromium.org/1855823003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:15: }, 'When
inserted into a document tree, isConnected should be true.');
Could you update the description string for all tests in this file
to match the changes?

[yuzuchan, 2016-04-05 08:29:53]

This is how I feel about this patch: http://i.imgur.com/WW0jdZv.gif

https://codereview.chromium.org/1855823003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html (right):

https://codereview.chromium.org/1855823003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shadow-dom/node-isConnected.html:15: }, 'When
inserted into a document tree, isConnected should be true.');
On 2016/04/05 at 07:06:39, kochi wrote:
> Could you update the description string for all tests in this file
> to match the changes?

Done.

[hayato, 2016-04-07 06:14:10]

LGTM

Since "Intent to Ship" [1] is LGTM3ed, you can land this patch now. :)

[1]:
https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/blink-dev/0...

[yuzuchan, 2016-04-08 04:34:55]

The CQ bit was checked by yuzus@chromium.org

[yuzuchan, 2016-04-08 04:34:55]

The patchset sent to the CQ was uploaded after l-g-t-m from hayato@chromium.org
Link to the patchset: https://codereview.chromium.org/1855823003/#ps100001
(title: "Change inDocument to inShadowIncludingDocument")

[commit-bot: I haz the power, 2016-04-08 04:35:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1855823003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1855823003/100001

[commit-bot: I haz the power, 2016-04-08 04:43:46]

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

[commit-bot: I haz the power, 2016-04-08 04:43:47]

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

[yuzuchan, 2016-04-08 04:45:10]

The CQ bit was checked by yuzus@chromium.org

[yuzuchan, 2016-04-08 04:45:13]

The CQ bit was unchecked by yuzus@chromium.org

[yuzuchan, 2016-04-08 04:45:26]

yuzus@chromium.org changed reviewers:
+ tkent@chromium.org

[yuzuchan, 2016-04-08 04:45:26]

PTAL :)

[tkent, 2016-04-08 04:56:01]

lgtm

[tkent, 2016-04-08 04:56:12]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2016-04-08 04:56:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1855823003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1855823003/100001

[commit-bot: I haz the power, 2016-04-08 05:01:10]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-08 05:02:35]

Description was changed from

==========
Implement Node.isConnected

This CL implements a Node atrribute Node.isConnected. Node.isConnected returns
true when a context object is in a shadow-including document, and false
otherwise.
See the spec here: https://dom.spec.whatwg.org/#dom-node-isconnected
See the discussion here: https://github.com/w3c/webcomponents/issues/81

BUG=599743
==========

to

==========
Implement Node.isConnected

This CL implements a Node atrribute Node.isConnected. Node.isConnected returns
true when a context object is in a shadow-including document, and false
otherwise.
See the spec here: https://dom.spec.whatwg.org/#dom-node-isconnected
See the discussion here: https://github.com/w3c/webcomponents/issues/81

BUG=599743

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

[commit-bot: I haz the power, 2016-04-08 05:02:36]

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