Closed Bug 1462703 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(3 files)

Bug 1462703 - Set returned CustomElementDefinition again after script runner is set
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1462703 - Upgrade the created element after callback runs
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1462703 - Additional setElementCreationCallback tests in XUL document
59 bytes, text/x-review-board-request
smaug
: review+
Details
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.
Attachment #8979837 - Flags: review?(bugs)
Attachment #8979838 - Flags: review?(bugs)
Attachment #8979839 - Flags: review?(bugs)
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 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 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 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+
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.

Attachment

General

Creator:
Created:
Updated:
Size: