Upgrade the custom element as soon as callback from customElements.setElementCreationCallback() returns a definition

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: timdream, Assigned: timdream)

Tracking

Trunk
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(3 attachments)

Updated

a year ago
Priority: -- → P3
Summary: Upgrade the custom element as soon callback from customElements.setElementCreationCallback() returns a definition → Upgrade the custom element as soon as callback from customElements.setElementCreationCallback() returns a definition
I've been working on this today and it ended up to be more complex than I want it too. I started by adding yet another type->elements map to the registry, and iterate through the elements when the definition is set. That part is done and working, yet it doesn't promise the execution order of custom element construction/upgrade is correct, like what bug 1326028 promises.

Considering how we will use setElementCreationCallback(), perhaps I would need to copy a few tests and set the CE definition with it, just to be sure that the behavior is entirely correct.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8979837 - Flags: review?(bugs)
Attachment #8979838 - Flags: review?(bugs)
Attachment #8979839 - Flags: review?(bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

a year ago
mozreview-review
Comment on attachment 8979837 [details]
Bug 1462703 - Set returned CustomElementDefinition again after script runner is set

https://reviewboard.mozilla.org/r/246006/#review252674

::: dom/base/CustomElementRegistry.cpp:336
(Diff revision 2)
>      mElementCreationCallbacks.Get(aTypeAtom, getter_AddRefs(callback));
>      if (callback) {
>        RefPtr<Runnable> runnable =
>          new RunCustomElementCreationCallback(this, aTypeAtom, callback);
>        nsContentUtils::AddScriptRunner(runnable);
>        mElementCreationCallbacks.Remove(aTypeAtom);

Don't you then want to move mElementCreationCallbacks.Remove(aTypeAtom); to be before AddScriptRunner.

and ok, if we can run the callback synchronously, https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/dom/base/nsContentUtils.cpp#10066 needs to be changed to be RefPtr to ensure registry doesn't get deleted by the callback call
Attachment #8979837 - Flags: review?(bugs) → review+

Comment 13

a year ago
mozreview-review
Comment on attachment 8979838 [details]
Bug 1462703 - Upgrade the created element after callback runs

https://reviewboard.mozilla.org/r/246008/#review252682

::: dom/base/CustomElementRegistry.h:458
(Diff revision 4)
>      DefinitionMap;
>    typedef nsRefPtrHashtable<nsRefPtrHashKey<nsAtom>, CustomElementCreationCallback>
>      ElementCreationCallbackMap;
>    typedef nsClassHashtable<nsRefPtrHashKey<nsAtom>,
>                             nsTHashtable<nsRefPtrHashKey<nsIWeakReference>>>
> -    CandidateMap;
> +    CustomTypeElementsMap;

I don't understand this naming.

::: dom/base/CustomElementRegistry.cpp:438
(Diff revision 4)
>  }
>  
> +void
> +CustomElementRegistry::RegisterCallbackUpgradeElement(Element* aElement,
> +                                                      nsAtom* aTypeName)
> +{

I think this needs to be faster method. Probably inline and return early if mElementCreationCallbacksUpgradeCandidatesMap is empty.
Attachment #8979838 - Flags: review?(bugs) → review-

Comment 14

a year ago
mozreview-review
Comment on attachment 8979839 [details]
Bug 1462703 - Additional setElementCreationCallback tests in XUL document

https://reviewboard.mozilla.org/r/246010/#review252688

::: dom/tests/mochitest/webcomponents/test_xul_custom_element.xul:320
(Diff revision 4)
>          is(Object.getPrototypeOf(element), XULElement.prototype,
>            `<${element.localName} is="${element.getAttribute("is")}" /> should not be a custom element.`);
>        }
>      }
>  
> +    function setElementCreationCallbackInDocument() {

This function doesn't set any element creation callback. Perhaps rename the function to tell what it is actually testing.
like, 
testSetElementCreationballbackInDocument
Attachment #8979839 - Flags: review?(bugs) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

a year ago
mozreview-review
Comment on attachment 8979838 [details]
Bug 1462703 - Upgrade the created element after callback runs

https://reviewboard.mozilla.org/r/246008/#review252838

::: dom/base/CustomElementRegistry.h:445
(Diff revisions 4 - 5)
> +    RefPtr<nsAtom> typeName = aTypeName;
> +    if (!typeName) {
> +      typeName = aElement->NodeInfo()->NameAtom();
> +    }
> +
> +    if (mElementCreationCallbacksUpgradeCandidatesMap.IsEmpty()) {

Move this to be the first thing in the method. No need to AddRef/Release atom if we aren't going to use it.

::: dom/base/nsContentUtils.cpp:10091
(Diff revisions 4 - 5)
>                                                 nsAtom* aTypeName)
>  {
>    MOZ_ASSERT(aElement);
>  
>    nsIDocument* doc = aElement->OwnerDoc();
> -  CustomElementRegistry* registry = GetCustomElementRegistry(doc);
> +  RefPtr<CustomElementRegistry> registry(GetCustomElementRegistry(doc));

This doesn't need to be RefPtr, as far as I see.
This is rather hot code path, so better to not do extra virtual calls.

::: dom/base/nsContentUtils.cpp:10103
(Diff revisions 4 - 5)
>  nsContentUtils::RegisterUnresolvedElement(Element* aElement, nsAtom* aTypeName)
>  {
>    MOZ_ASSERT(aElement);
>  
>    nsIDocument* doc = aElement->OwnerDoc();
> -  CustomElementRegistry* registry = GetCustomElementRegistry(doc);
> +  RefPtr<CustomElementRegistry> registry(GetCustomElementRegistry(doc));

This doesn't need to be RefPtr, as far as I see.
This is rather hot code path, so better to not do extra virtual calls.
Attachment #8979838 - Flags: review?(bugs) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

a year ago
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/034c16b7efa2
Set returned CustomElementDefinition again after script runner is set r=smaug
https://hg.mozilla.org/integration/autoland/rev/af3d1bc9c492
Upgrade the created element after callback runs r=smaug
https://hg.mozilla.org/integration/autoland/rev/60a74d5d7904
Additional setElementCreationCallback tests in XUL document r=smaug
You need to log in before you can comment on or make changes to this bug.