Allow attachShadow only for elements in the safelist

Allow attachShadow only for elements in the safelist

The spec is: http://w3c.github.io/webcomponents/spec/shadow/#methods-1
The spec discussion is: https://github.com/w3c/webcomponents/issues/110

BUG=531990
Committed: https://crrev.com/ab5dc4686858b7d8e90d78247365325be61a9e90
Cr-Commit-Position: refs/heads/master@{#375539}

[hayato, 2016-02-16 05:57:20]

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

[commit-bot: I haz the power, 2016-02-16 05:57:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1698183003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1698183003/1

[hayato, 2016-02-16 05:57:55]

Description was changed from

==========
Allow attachShadow only for element in the safelist

The spec is: http://w3c.github.io/webcomponents/spec/shadow/#methods-1
The spec discussion is: https://github.com/w3c/webcomponents/issues/110

BUG=531990
==========

to

==========
Allow attachShadow only for elements in the safelist

The spec is: http://w3c.github.io/webcomponents/spec/shadow/#methods-1
The spec discussion is: https://github.com/w3c/webcomponents/issues/110

BUG=531990
==========

[hayato, 2016-02-16 06:01:14]

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

[hayato, 2016-02-16 06:01:15]

wip

[hayato, 2016-02-16 06:02:46]

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

[hayato, 2016-02-16 06:02:48]

fix

[commit-bot: I haz the power, 2016-02-16 06:02:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1698183003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1698183003/20001

[hayato, 2016-02-16 06:04:34]

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

[hayato, 2016-02-16 06:04:35]

fix

[commit-bot: I haz the power, 2016-02-16 06:04:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1698183003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1698183003/60001

[hayato, 2016-02-16 06:20:18]

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

[hayato, 2016-02-16 06:20:21]

rebased

[commit-bot: I haz the power, 2016-02-16 06:20:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1698183003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1698183003/80001

[hayato, 2016-02-16 06:31:31]

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

[hayato, 2016-02-16 06:31:31]

PTAL

[kochi, 2016-02-16 06:56:13]

https://codereview.chromium.org/1698183003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1698183003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1917: bool tagNameIsSupported =
isCustomElement()
Can you add a reference URL as a comment?

https://codereview.chromium.org/1698183003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1917: bool tagNameIsSupported =
isCustomElement()
Is isCustomElement() guaranteed to work when attachShadow() is used in
createdCallback?

[hayato, 2016-02-16 07:11:14]

Thank you for the review.

https://codereview.chromium.org/1698183003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1698183003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1917: bool tagNameIsSupported =
isCustomElement()
On 2016/02/16 06:56:13, kochi wrote:
> Is isCustomElement() guaranteed to work when attachShadow() is used in
> createdCallback?

Could you elaborate the case? I do not understand the point.

https://codereview.chromium.org/1698183003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1917: bool tagNameIsSupported =
isCustomElement()
On 2016/02/16 06:56:13, kochi wrote:
> Can you add a reference URL as a comment?

I do not think we need to refer it.
It is obvious that this safelist is defined in the spec of Element.attachShadow
from the name of Element::attachShadow.

[commit-bot: I haz the power, 2016-02-16 07:17:47]

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

[commit-bot: I haz the power, 2016-02-16 07:17:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kochi, 2016-02-16 07:28:15]

https://codereview.chromium.org/1698183003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1698183003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1917: bool tagNameIsSupported =
isCustomElement()
On 2016/02/16 07:11:13, hayato wrote:
> On 2016/02/16 06:56:13, kochi wrote:
> > Can you add a reference URL as a comment?
> 
> I do not think we need to refer it.
> It is obvious that this safelist is defined in the spec of
Element.attachShadow
> from the name of Element::attachShadow.

Acknowledged.

https://codereview.chromium.org/1698183003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1917: bool tagNameIsSupported =
isCustomElement()
On 2016/02/16 07:11:13, hayato wrote:
> On 2016/02/16 06:56:13, kochi wrote:
> > Is isCustomElement() guaranteed to work when attachShadow() is used in
> > createdCallback?
> 
> Could you elaborate the case? I do not understand the point.

I thought a real custom element is an element which is already upgraded.
Do we allow attaching a shadow on arbitrary element with name consisting '-'
but which is not really a custom element yet?

Mentioning createdCallback was confusing.
I should have meant "a element without document.registerElement (or
defineElement)".

I guess it's safe to allow for such elements who are waiting for upgrade, but I
may
miss something if we allow attaching a shadow before upgrading, so I wanted to
make
sure we are safe to allow attaching shadow for those elements.

[hayato, 2016-02-16 07:46:34]

On 2016/02/16 07:28:15, kochi wrote:
>
https://codereview.chromium.org/1698183003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Element.cpp (right):
> 
>
https://codereview.chromium.org/1698183003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Element.cpp:1917: bool tagNameIsSupported =
> isCustomElement()
> On 2016/02/16 07:11:13, hayato wrote:
> > On 2016/02/16 06:56:13, kochi wrote:
> > > Can you add a reference URL as a comment?
> > 
> > I do not think we need to refer it.
> > It is obvious that this safelist is defined in the spec of
> Element.attachShadow
> > from the name of Element::attachShadow.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1698183003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Element.cpp:1917: bool tagNameIsSupported =
> isCustomElement()
> On 2016/02/16 07:11:13, hayato wrote:
> > On 2016/02/16 06:56:13, kochi wrote:
> > > Is isCustomElement() guaranteed to work when attachShadow() is used in
> > > createdCallback?
> > 
> > Could you elaborate the case? I do not understand the point.
> 
> I thought a real custom element is an element which is already upgraded.
> Do we allow attaching a shadow on arbitrary element with name consisting '-'
> but which is not really a custom element yet?
> 
> Mentioning createdCallback was confusing.
> I should have meant "a element without document.registerElement (or
> defineElement)".
> 
> I guess it's safe to allow for such elements who are waiting for upgrade, but
I
> may
> miss something if we allow attaching a shadow before upgrading, so I wanted to
> make
> sure we are safe to allow attaching shadow for those elements.

I got it. We should allow it. That's my intention. Maybe the spec should be
clarified with that.

[kochi, 2016-02-16 07:54:55]

On 2016/02/16 07:46:34, hayato wrote:
> On 2016/02/16 07:28:15, kochi wrote:
> >
>
https://codereview.chromium.org/1698183003/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/dom/Element.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1698183003/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/dom/Element.cpp:1917: bool tagNameIsSupported
=
> > isCustomElement()
> > On 2016/02/16 07:11:13, hayato wrote:
> > > On 2016/02/16 06:56:13, kochi wrote:
> > > > Can you add a reference URL as a comment?
> > > 
> > > I do not think we need to refer it.
> > > It is obvious that this safelist is defined in the spec of
> > Element.attachShadow
> > > from the name of Element::attachShadow.
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.chromium.org/1698183003/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/dom/Element.cpp:1917: bool tagNameIsSupported
=
> > isCustomElement()
> > On 2016/02/16 07:11:13, hayato wrote:
> > > On 2016/02/16 06:56:13, kochi wrote:
> > > > Is isCustomElement() guaranteed to work when attachShadow() is used in
> > > > createdCallback?
> > > 
> > > Could you elaborate the case? I do not understand the point.
> > 
> > I thought a real custom element is an element which is already upgraded.
> > Do we allow attaching a shadow on arbitrary element with name consisting '-'
> > but which is not really a custom element yet?
> > 
> > Mentioning createdCallback was confusing.
> > I should have meant "a element without document.registerElement (or
> > defineElement)".
> > 
> > I guess it's safe to allow for such elements who are waiting for upgrade,
but
> I
> > may
> > miss something if we allow attaching a shadow before upgrading, so I wanted
to
> > make
> > sure we are safe to allow attaching shadow for those elements.
> 
> I got it. We should allow it. That's my intention. Maybe the spec should be
> clarified with that.

Thanks, lgtm then!

It looks
imported/web-platform-tests/shadow-dom/Element-interface-attachShadow.html is
passing with this change (which is good!)

Could you remove
imported/web-platform-tests/shadow-dom/Element-interface-attachShadow-expected.txt
in this CL?
The only failing test is the test that passed with this change, so we no longer
need the
-expected.txt file.

[hayato, 2016-02-16 08:16:42]

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

[hayato, 2016-02-16 08:16:45]

Remove expected.txt

[commit-bot: I haz the power, 2016-02-16 08:17:52]

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

[kochi, 2016-02-16 08:18:00]

lgtm

[hayato, 2016-02-16 08:20:02]

The CQ bit was unchecked by hayato@chromium.org

[hayato, 2016-02-16 08:20:02]

The CQ bit was checked by hayato@chromium.org

[commit-bot: I haz the power, 2016-02-16 08:20:49]

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

[commit-bot: I haz the power, 2016-02-16 09:26:36]

Description was changed from

==========
Allow attachShadow only for elements in the safelist

The spec is: http://w3c.github.io/webcomponents/spec/shadow/#methods-1
The spec discussion is: https://github.com/w3c/webcomponents/issues/110

BUG=531990
==========

to

==========
Allow attachShadow only for elements in the safelist

The spec is: http://w3c.github.io/webcomponents/spec/shadow/#methods-1
The spec discussion is: https://github.com/w3c/webcomponents/issues/110

BUG=531990
==========

[commit-bot: I haz the power, 2016-02-16 09:26:37]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-16 22:51:58]

Description was changed from

==========
Allow attachShadow only for elements in the safelist

The spec is: http://w3c.github.io/webcomponents/spec/shadow/#methods-1
The spec discussion is: https://github.com/w3c/webcomponents/issues/110

BUG=531990
==========

to

==========
Allow attachShadow only for elements in the safelist

The spec is: http://w3c.github.io/webcomponents/spec/shadow/#methods-1
The spec discussion is: https://github.com/w3c/webcomponents/issues/110

BUG=531990

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

[commit-bot: I haz the power, 2016-02-16 22:51:58]

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