Bug 183397 - Implement customElements.upgrade()
Summary: Implement customElements.upgrade()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
: 196165 (view as bug list)
Depends on:
Blocks: 154907
  Show dependency tree / graph
 
Reported: 2018-03-06 21:27 PST by Domenic Denicola
Modified: 2019-03-23 23:37 PDT (History)
11 users (show)

See Also:

Attachments
Implements the feature (7.85 KB, patch)
2018-08-01 23:37 PDT, Ryosuke Niwa 		fred.wang: review+
Details | Formatted Diff | Diff
Added more descriptions to the change log (8.35 KB, patch)
2018-08-01 23:50 PDT, Ryosuke Niwa 		fred.wang: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Radar WebKit Bug Importer 2018-03-08 23:09:19 PST 
<rdar://problem/38292356>
Comment 2 Ryosuke Niwa 2018-08-01 23:37:34 PDT 
Created attachment 346361 [details]
Implements the feature
Comment 3 Ryosuke Niwa 2018-08-01 23:43:18 PDT 
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.
Comment 4 Ryosuke Niwa 2018-08-01 23:50:36 PDT 
Created attachment 346362 [details]
Added more descriptions to the change log
Comment 5 Frédéric Wang (:fredw) 2018-08-02 00:50:30 PDT 
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.
Comment 6 Ryosuke Niwa 2018-08-02 12:23:47 PDT 
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.
Comment 7 Ryosuke Niwa 2018-08-02 12:25:25 PDT 
Committed r234507: <https://trac.webkit.org/changeset/234507>
Comment 8 Ryosuke Niwa 2019-03-23 23:37:36 PDT 
*** Bug 196165 has been marked as a duplicate of this bug. ***