Bug 183397 – Implement customElements.upgrade()

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Domenic Denicola]

Recently added to the spec: https://github.com/whatwg/html/pull/3535

Spec: https://html.spec.whatwg.org/multipage/custom-elements.html#dom-customelementregistry-upgrade

Tests: https://github.com/w3c/web-platform-tests/pull/9869

[Radar WebKit Bug Importer]

<rdar://problem/38292356>

[Ryosuke Niwa]

Created attachment 346361 [details]
Implements the feature

[Ryosuke Niwa]

Comment on attachment 346361 [details]
Implements the feature

View in context: https://bugs.webkit.org/attachment.cgi?id=346361&action=review

> Source/WebCore/dom/CustomElementReactionQueue.cpp:-125
> -    ASSERT(element.isConnected());

We need to remove this since upgrade can be called on a disconnected tree, and that's sort of the whole point of this API.

[Ryosuke Niwa]

Created attachment 346362 [details]
Added more descriptions to the change log

[Frédéric Wang (:fredw)]

Comment on attachment 346361 [details]
Implements the feature

View in context: https://bugs.webkit.org/attachment.cgi?id=346361&action=review

>> Source/WebCore/dom/CustomElementReactionQueue.cpp:-125
>> -    ASSERT(element.isConnected());
> 
> We need to remove this since upgrade can be called on a disconnected tree, and that's sort of the whole point of this API.

Right. It seems Element::insertedIntoAncestor is the only place where enqueueElementUpgradeIfDefined was called.

> Source/WebCore/dom/CustomElementRegistry.cpp:117
> +static void upgradeElementsInShadowIncludingdescendants(ContainerNode& root)

nit: 'D' of descendants should be uppercase.

> Source/WebCore/dom/CustomElementRegistry.h:44
> +class DeferredPromise;

The DeferredPromise include / forward declaration change seems unrelated to this patch, right? Maybe mention in the ChangeLog whether it is just a fix passing by.

[Ryosuke Niwa]

Thanks for the review!

(In reply to Frédéric Wang (:fredw) from comment #5)
> Comment on attachment 346361 [details]
> Implements the feature
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346361&action=review
> 
> >> Source/WebCore/dom/CustomElementReactionQueue.cpp:-125
> >> -    ASSERT(element.isConnected());
> > 
> > We need to remove this since upgrade can be called on a disconnected tree, and that's sort of the whole point of this API.
> 
> Right. It seems Element::insertedIntoAncestor is the only place where
> enqueueElementUpgradeIfDefined was called.
> 
> > Source/WebCore/dom/CustomElementRegistry.cpp:117
> > +static void upgradeElementsInShadowIncludingdescendants(ContainerNode& root)
> 
> nit: 'D' of descendants should be uppercase.

Fixed.

> > Source/WebCore/dom/CustomElementRegistry.h:44
> > +class DeferredPromise;
> 
> The DeferredPromise include / forward declaration change seems unrelated to
> this patch, right? Maybe mention in the ChangeLog whether it is just a fix
> passing by.

Sure, I'd add a change log comment.

[Ryosuke Niwa]

Committed r234507: <https://trac.webkit.org/changeset/234507>

[Ryosuke Niwa]

*** Bug 196165 has been marked as a duplicate of this bug. ***