Fix upgrading built-in custom elements

Fix upgrading built-in custom elements

Currently, the built-in elements fail to be tried to upgrade
and are not added to upgrade candidates list

BUG=618606

[Anton Obzhirov, 2017-06-25 16:11:53]

Description was changed from

==========
Fix upgrading built-in custom elements

Currently, the built-in elements fail to be tried to upgrade
and are not added to upgrade candidates list

BUG=618606
==========

to

==========
Fix upgrading built-in custom elements

Currently, the built-in elements fail to be tried to upgrade
and are not added to upgrade candidates list

BUG=618606
==========

[Anton Obzhirov, 2017-06-25 16:11:53]

a.obzhirov@samsung.com changed reviewers:
+ dominicc@chromium.org

[dominicc (has gone to gerrit), 2017-06-26 02:59:30]

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

[commit-bot: I haz the power, 2017-06-26 02:59:44]

Dry run: CQ is trying da patch.

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

[dominicc (has gone to gerrit), 2017-06-26 03:03:19]

Thanks for working on this. I think this needs work, some suggestions inline.

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

https://codereview.chromium.org/2954843002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.cpp:1452: if
(RuntimeEnabledFeatures::CustomElementsBuiltinEnabled() &&
I thought the timing of processing these was different for customized built-in
elements in V1. Can you put a link to the spec algorithm and note which step
does this?

https://codereview.chromium.org/2954843002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/custom/CustomElementRegistry.cpp (left):

https://codereview.chromium.org/2954843002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementRegistry.cpp:273: const
AtomicString& name = candidate->localName();
I think you should structure this differently, because <paper-button
is="dom-repeat"> should be a paper-button, not a dom-repeat. Doing it this way
makes it fragile because it subtly relies on the caller calling AddCandidate
before the attribute is set, but before the attribute is changed, and only if
the element name is not a custom element name.

I'm not confident those conditions hold.

Could you add some tests which try to mix autonomous custom elements and
is="..." attributes?

[commit-bot: I haz the power, 2017-06-26 04:55:26]

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

[commit-bot: I haz the power, 2017-06-26 04:55:27]

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

[Anton Obzhirov, 2017-06-27 19:19:00]

Thanks for the review, will update the patch.

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

https://codereview.chromium.org/2954843002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.cpp:1452: if
(RuntimeEnabledFeatures::CustomElementsBuiltinEnabled() &&
On 2017/06/26 03:03:19, dominicc (has gone to gerrit) wrote:
> I thought the timing of processing these was different for customized built-in
> elements in V1. Can you put a link to the spec algorithm and note which step
> does this?

Well it is wrong place to set the state, you are right.

The state is supposed to be set to 'undefined' for new element
if 'is' is not null and definition is null.
See https://dom.spec.whatwg.org/#concept-create-element,
7.3. If namespace is the HTML namespace, and either localName is a valid custom
element name or is is non-null, then set result’s custom element state to
"undefined".

So I think the problem is really in HTMLElementFactory::createHTMLElement so it
doesn't create built-in element through CustomElement::CreateCustomElementSync
API. I will make the changes.

https://codereview.chromium.org/2954843002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/custom/CustomElementRegistry.cpp (left):

https://codereview.chromium.org/2954843002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementRegistry.cpp:273: const
AtomicString& name = candidate->localName();
On 2017/06/26 03:03:19, dominicc (has gone to gerrit) wrote:
> I think you should structure this differently, because <paper-button
> is="dom-repeat"> should be a paper-button, not a dom-repeat. Doing it this way
> makes it fragile because it subtly relies on the caller calling AddCandidate
> before the attribute is set, but before the attribute is changed, and only if
> the element name is not a custom element name.
> 
> I'm not confident those conditions hold.
> 
> Could you add some tests which try to mix autonomous custom elements and
> is="..." attributes?

Yes, will do, haven’t realized you could mix both.

[Anton Obzhirov, 2017-06-28 14:09:07]

https://codereview.chromium.org/2954843002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/custom/CustomElementRegistry.cpp (left):

https://codereview.chromium.org/2954843002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementRegistry.cpp:273: const
AtomicString& name = candidate->localName();
On 2017/06/27 19:19:00, Anton Obzhirov wrote:
> On 2017/06/26 03:03:19, dominicc (has gone to gerrit) wrote:
> > I think you should structure this differently, because <paper-button
> > is="dom-repeat"> should be a paper-button, not a dom-repeat. Doing it this
way
> > makes it fragile because it subtly relies on the caller calling AddCandidate
> > before the attribute is set, but before the attribute is changed, and only
if
> > the element name is not a custom element name.
> > 
> > I'm not confident those conditions hold.
> > 
> > Could you add some tests which try to mix autonomous custom elements and
> > is="..." attributes?
> 
> Yes, will do, haven’t realized you could mix both.
BTW according to
https://html.spec.whatwg.org/multipage/custom-elements.html#look-up-a-custom-...

7.1 If extends is a valid custom element name, then throw a "NotSupportedError"
DOMException.
So it shouldn’t be possible to extend autonomous elements via 'is' attribute.
Plz correct me if I am wrong.
In this case 'is' should always override local name. But I would say
AddCandidate should accept CustomElementDescriptor as a second parameter.

[dominicc (has gone to gerrit), 2017-06-29 05:39:05]

On 2017/06/28 at 14:09:07, a.obzhirov wrote:
>
https://codereview.chromium.org/2954843002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/custom/CustomElementRegistry.cpp
(left):
> 
>
https://codereview.chromium.org/2954843002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/custom/CustomElementRegistry.cpp:273: const
AtomicString& name = candidate->localName();
> On 2017/06/27 19:19:00, Anton Obzhirov wrote:
> > On 2017/06/26 03:03:19, dominicc (has gone to gerrit) wrote:
> > > I think you should structure this differently, because <paper-button
> > > is="dom-repeat"> should be a paper-button, not a dom-repeat. Doing it this
way
> > > makes it fragile because it subtly relies on the caller calling
AddCandidate
> > > before the attribute is set, but before the attribute is changed, and only
if
> > > the element name is not a custom element name.
> > > 
> > > I'm not confident those conditions hold.
> > > 
> > > Could you add some tests which try to mix autonomous custom elements and
> > > is="..." attributes?
> > 
> > Yes, will do, haven’t realized you could mix both.
> BTW according to
https://html.spec.whatwg.org/multipage/custom-elements.html#look-up-a-custom-...

> 7.1 If extends is a valid custom element name, then throw a
"NotSupportedError" DOMException.
> So it shouldn’t be possible to extend autonomous elements via 'is' attribute.
Plz correct me if I am wrong.
> In this case 'is' should always override local name. But I would say
> AddCandidate should accept CustomElementDescriptor as a second parameter.

Yes, exactly. You need to test that interaction works not only when defining but
also when creating.

[Anton Obzhirov, 2017-06-29 07:07:20]

I've uploaded new version to
https://chromium-review.googlesource.com/547715
Plz have a look, I've done some refactoring. The change started to get bigger
then I expected :) Please let me know your thoughts.