1392970 - Consider having a reference of the CustomElementDefinition in the custom element itself

Status: RESOLVED FIXED

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

Description

[Jessica Jong [:jessica] (inactive)]

Now, we store the list of CustomElementDefinitions in CustomElementRegistry, but we need to go through the list every time when want to get the custom element definition of a given element.
It'd save us a lot of time if we keep a reference of the CustomElementDefinition in the custom element itself.

Comment 1

[Jessica Jong [:jessica] (inactive)]

The idea is to put CustomElementDefinition in CustomElementData. CustomElementData is created when an element might be a custom element: has a valid custom element name or has an 'is' attribute. There may be the case where the CustomElementData is created without having a CustomElementDefinition, but we can always fill in the CustomElementDefinition when the custom element is later upgraded.

Right now, we put the custom element state [1] 'mState' in CustomElementData (which is stored in an element's extended slots), I wonder whether we should move this to Element since a non-custom element may not have a CustomElementData but its state should be 'uncustomized'.


[1] https://dom.spec.whatwg.org/#concept-element-custom-element-state

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: Make CE Definition ref-counted and put in in CE Data.

Hi Olli, this is just the first step, may I have your early feedback? especially on those CC stuff. Thanks.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8902227 [details] [diff] [review]
Part 1: Make CE Definition ref-counted and put in in CE Data.

>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(CustomElementDefinition)
>+  auto& callbacks = tmp->mCallbacks;
I know you just moved the code here, but I wouldn't mind remove use of auto& and use the
actual type. Would make reading the code easier.
>+  void
>+  SetCustomElementDefinition(CustomElementDefinition* aDefinition)
>+  {
>+    MOZ_ASSERT(!mCustomElementDefinition);
>+
>+    mCustomElementDefinition = aDefinition;
>+  }
>+
>+  CustomElementDefinition*
>+  GetCustomElementDefinition()
>+  {
>+    if (mState != State::eCustom) {
>+      return nullptr;
>+    }
>+
>+    return mCustomElementDefinition;
This could use a comment. How state can be non-custom, yet mCustomElementDefinition non-null?

>+  void Unlink()
>+  {
>+    CustomElementDefinition* tmp = this;
>+    NS_IMPL_CYCLE_COLLECTION_UNLINK(mConstructor)
>+    mPrototype = nullptr;
>+    mCallbacks = nullptr;
>+  }
It is unclear why we need this, and why this isn't in _UNLINK
nsDOMSlots::Unlink should be able to just set the mCustomElementDefinition to null.


>+CustomElementDefinition*
>+Element::GetCustomElementDefinition() const
>+{
>+  CustomElementData* data = GetCustomElementData();
>+  MOZ_ASSERT(data);
GetCustomElementData() may return null, so this probably shouldn't just assert.


>+Element::SetCustomElementDefinition(CustomElementDefinition* aDefinition)
>+{
>+  MOZ_ASSERT(aDefinition);
>+
>+  CustomElementData* data = GetCustomElementData();
>+  MOZ_ASSERT(data);
Same here


>+  /**
>+   * Gets the custom element definition used by web components custom element.
>+   *
>+   * @return The custom element definition or null if custom element state is
>+   *         not 'custom' or custom element is not defined yet.
>+   */
>+  CustomElementDefinition* GetCustomElementDefinition() const;
And this even documents that null is returned if state isn't custom.
>   if (mExtendedSlots->mCustomElementData) {
>     for (uint32_t i = 0;
>          i < mExtendedSlots->mCustomElementData->mReactionQueue.Length(); i++) {
>       if (mExtendedSlots->mCustomElementData->mReactionQueue[i]) {
>         mExtendedSlots->mCustomElementData->mReactionQueue[i]->Traverse(cb);
>       }
>     }
>+
>+    if (mExtendedSlots->mCustomElementData->mCustomElementDefinition) {
>+      NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb,
>+        "mExtendedSlots->mCustomElementData->mCustomElementDefinition");
>+      cb.NoteNativeChild(mExtendedSlots->mCustomElementData->mCustomElementDefinition,
>+        NS_CYCLE_COLLECTION_PARTICIPANT(CustomElementDefinition));
>+    }
>   }
> 
>   for (auto iter = mExtendedSlots->mRegisteredIntersectionObservers.Iter();
>        !iter.Done(); iter.Next()) {
>     DOMIntersectionObserver* observer = iter.Key();
>     NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb,
>                                        "mExtendedSlots->mRegisteredIntersectionObservers[i]");
>     cb.NoteXPCOMChild(observer);
>@@ -844,16 +851,22 @@ FragmentOrElement::nsDOMSlots::Unlink()
>   mExtendedSlots->mSMILOverrideStyle = nullptr;
>   mExtendedSlots->mControllers = nullptr;
>   mExtendedSlots->mLabelsList = nullptr;
>   mExtendedSlots->mShadowRoot = nullptr;
>   mExtendedSlots->mContainingShadow = nullptr;
>   MOZ_ASSERT(!(mExtendedSlots->mXBLBinding));
>   mExtendedSlots->mXBLInsertionParent = nullptr;
>   mExtendedSlots->mCustomElementData = nullptr;
>+  if (mExtendedSlots->mCustomElementData) {
This can't be right.
You first set mExtendedSlots->mCustomElementData to null and then check that it isn't null ;)

Comment 4

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 8902227 [details] [diff] [review]
> Part 1: Make CE Definition ref-counted and put in in CE Data.
> 
> >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(CustomElementDefinition)
> >+  auto& callbacks = tmp->mCallbacks;
> I know you just moved the code here, but I wouldn't mind remove use of auto&
> and use the
> actual type. Would make reading the code easier.

Sure, will do.

> >+  void
> >+  SetCustomElementDefinition(CustomElementDefinition* aDefinition)
> >+  {
> >+    MOZ_ASSERT(!mCustomElementDefinition);
> >+
> >+    mCustomElementDefinition = aDefinition;
> >+  }
> >+
> >+  CustomElementDefinition*
> >+  GetCustomElementDefinition()
> >+  {
> >+    if (mState != State::eCustom) {
> >+      return nullptr;
> >+    }
> >+
> >+    return mCustomElementDefinition;
> This could use a comment. How state can be non-custom, yet
> mCustomElementDefinition non-null?

Ok, will add some comments here.

> 
> >+  void Unlink()
> >+  {
> >+    CustomElementDefinition* tmp = this;
> >+    NS_IMPL_CYCLE_COLLECTION_UNLINK(mConstructor)
> >+    mPrototype = nullptr;
> >+    mCallbacks = nullptr;
> >+  }
> It is unclear why we need this, and why this isn't in _UNLINK
> nsDOMSlots::Unlink should be able to just set the mCustomElementDefinition
> to null.

I see, I'll remove this function and move these lines in _UNLINK.

> 
> 
> >+CustomElementDefinition*
> >+Element::GetCustomElementDefinition() const
> >+{
> >+  CustomElementData* data = GetCustomElementData();
> >+  MOZ_ASSERT(data);
> GetCustomElementData() may return null, so this probably shouldn't just
> assert.

You're right, we should just return null if there is no CustomElementData.

> 
> 
> >+Element::SetCustomElementDefinition(CustomElementDefinition* aDefinition)
> >+{
> >+  MOZ_ASSERT(aDefinition);
> >+
> >+  CustomElementData* data = GetCustomElementData();
> >+  MOZ_ASSERT(data);
> Same here

I'd prefer to keep the assertion here, if we set a CE definition to a non-custom element, that's probably wrong and there should only be a few places where we set the the custom element definition to an element.

> 
> 
> >+  /**
> >+   * Gets the custom element definition used by web components custom element.
> >+   *
> >+   * @return The custom element definition or null if custom element state is
> >+   *         not 'custom' or custom element is not defined yet.
> >+   */
> >+  CustomElementDefinition* GetCustomElementDefinition() const;
> And this even documents that null is returned if state isn't custom.
> >   if (mExtendedSlots->mCustomElementData) {
> >     for (uint32_t i = 0;
> >          i < mExtendedSlots->mCustomElementData->mReactionQueue.Length(); i++) {
> >       if (mExtendedSlots->mCustomElementData->mReactionQueue[i]) {
> >         mExtendedSlots->mCustomElementData->mReactionQueue[i]->Traverse(cb);
> >       }
> >     }
> >+
> >+    if (mExtendedSlots->mCustomElementData->mCustomElementDefinition) {
> >+      NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb,
> >+        "mExtendedSlots->mCustomElementData->mCustomElementDefinition");
> >+      cb.NoteNativeChild(mExtendedSlots->mCustomElementData->mCustomElementDefinition,
> >+        NS_CYCLE_COLLECTION_PARTICIPANT(CustomElementDefinition));
> >+    }
> >   }
> > 
> >   for (auto iter = mExtendedSlots->mRegisteredIntersectionObservers.Iter();
> >        !iter.Done(); iter.Next()) {
> >     DOMIntersectionObserver* observer = iter.Key();
> >     NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb,
> >                                        "mExtendedSlots->mRegisteredIntersectionObservers[i]");
> >     cb.NoteXPCOMChild(observer);
> >@@ -844,16 +851,22 @@ FragmentOrElement::nsDOMSlots::Unlink()
> >   mExtendedSlots->mSMILOverrideStyle = nullptr;
> >   mExtendedSlots->mControllers = nullptr;
> >   mExtendedSlots->mLabelsList = nullptr;
> >   mExtendedSlots->mShadowRoot = nullptr;
> >   mExtendedSlots->mContainingShadow = nullptr;
> >   MOZ_ASSERT(!(mExtendedSlots->mXBLBinding));
> >   mExtendedSlots->mXBLInsertionParent = nullptr;
> >   mExtendedSlots->mCustomElementData = nullptr;
> >+  if (mExtendedSlots->mCustomElementData) {
> This can't be right.
> You first set mExtendedSlots->mCustomElementData to null and then check that
> it isn't null ;)

Oops, I meant to move that line into the if statement.

Comment 5

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jessica] from comment #4)
> (In reply to Olli Pettay [:smaug] from comment #3)
> > >+  void
> > >+  SetCustomElementDefinition(CustomElementDefinition* aDefinition)
> > >+  {
> > >+    MOZ_ASSERT(!mCustomElementDefinition);
> > >+
> > >+    mCustomElementDefinition = aDefinition;
> > >+  }
> > >+
> > >+  CustomElementDefinition*
> > >+  GetCustomElementDefinition()
> > >+  {
> > >+    if (mState != State::eCustom) {
> > >+      return nullptr;
> > >+    }
> > >+
> > >+    return mCustomElementDefinition;
> > This could use a comment. How state can be non-custom, yet
> > mCustomElementDefinition non-null?
> 
> Ok, will add some comments here.
> 

Actually, we only set custom element definition when custom element state is eCustom, mCustomElementDefinition should be null in other states, so I guess I can just return mCustomElementDefinition here.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Looks like we can't change to use CustomElementDefinition in CustomElementData just yet, since CustomElementDefinition will be put into CustomElementData when:
- upgrading
- running html constructor
And bug 1301024 is in charge of calling the above functions at the correct time for createElement/createElementNS. So using Element::GetCustomElementDefinition will work correctly only after bug 1301024.

I can land the part where we set CustomElementDefinition though...

Comment 7

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: Make CE Definition ref-counted and put in in CE Data, v1.

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: Get CE Defintion from CE Data when possible, v1.

Comment 9

[Jessica Jong [:jessica] (inactive)]

I'd like to get the patches reviewed first but they need to land AFTER bug 1301024.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8904291 [details] [diff] [review]
Part 2: Get CE Defintion from CE Data when possible, v1.

># HG changeset patch
># User Jessica Jong <[e-mail]>
># Parent  07c702fd588696cdb0ea534e6800ee28918d97eb
>Bug 1392970 - Part 2: Get CustomElementDefinition from CustomElementData when possible. r=smaug
>
>MozReview-Commit-ID: L6DCtGXBesX
>
>diff --git a/dom/base/CustomElementRegistry.cpp b/dom/base/CustomElementRegistry.cpp
>--- a/dom/base/CustomElementRegistry.cpp
>+++ b/dom/base/CustomElementRegistry.cpp
>@@ -31,18 +31,30 @@ CustomElementCallback::Call()
>       // this now in order to enqueue the attached callback. This is a spec
>       // bug (w3c bug 27437).
>       mOwnerData->mCreatedCallbackInvoked = true;
> 
>       // If ELEMENT is in a document and this document has a browsing context,
>       // enqueue attached callback for ELEMENT.
>       nsIDocument* document = mThisObject->GetComposedDoc();
>       if (document && document->GetDocShell()) {
>+        NodeInfo *ni = mThisObject->NodeInfo();
Nit, * goes with the type

>+        nsString extType = nsDependentAtomString(mOwnerData->mType);
nsDependentAtomString extType(mOwnerData->mType);
that way you avoid creating temporary heap allocated string.
Or does that not compile?


>+
>+        // We need to do this because at this point, CustomElementDefinition is
>+        // not set to CustomElementData yet, so EnqueueLifecycleCallback will
>+        // fail to find the CE definition for this custom element.
>+        // This way go away eventually since there is no created callback in v1.
s/way/may/ ?

>+        CustomElementDefinition *definition =
Nit, * with the type

Comment 11

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #10)
> Comment on attachment 8904291 [details] [diff] [review]
> Part 2: Get CE Defintion from CE Data when possible, v1.
> 
> ># HG changeset patch
> ># User Jessica Jong <[e-mail]>
> ># Parent  07c702fd588696cdb0ea534e6800ee28918d97eb
> >Bug 1392970 - Part 2: Get CustomElementDefinition from CustomElementData when possible. r=smaug
> >
> >MozReview-Commit-ID: L6DCtGXBesX
> >
> >diff --git a/dom/base/CustomElementRegistry.cpp b/dom/base/CustomElementRegistry.cpp
> >--- a/dom/base/CustomElementRegistry.cpp
> >+++ b/dom/base/CustomElementRegistry.cpp
> >@@ -31,18 +31,30 @@ CustomElementCallback::Call()
> >       // this now in order to enqueue the attached callback. This is a spec
> >       // bug (w3c bug 27437).
> >       mOwnerData->mCreatedCallbackInvoked = true;
> > 
> >       // If ELEMENT is in a document and this document has a browsing context,
> >       // enqueue attached callback for ELEMENT.
> >       nsIDocument* document = mThisObject->GetComposedDoc();
> >       if (document && document->GetDocShell()) {
> >+        NodeInfo *ni = mThisObject->NodeInfo();
> Nit, * goes with the type

Sorry that you need to keep correcting me about this. :(

> 
> >+        nsString extType = nsDependentAtomString(mOwnerData->mType);
> nsDependentAtomString extType(mOwnerData->mType);
> that way you avoid creating temporary heap allocated string.
> Or does that not compile?

Yes, it compiles, will change it in the final patch.

> 
> 
> >+
> >+        // We need to do this because at this point, CustomElementDefinition is
> >+        // not set to CustomElementData yet, so EnqueueLifecycleCallback will
> >+        // fail to find the CE definition for this custom element.
> >+        // This way go away eventually since there is no created callback in v1.
> s/way/may/ ?

Actually, I meant to write: "This will go away...", ha.

> 
> >+        CustomElementDefinition *definition =
> Nit, * with the type

Will do.

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: Make CE Definition ref-counted and put in in CE Data, v1.

rebased.

Comment 13

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: Get CE Defintion from CE Data when possible, v2.

Rebased and addressed review comment 10.

Comment 14

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3000ad937c1c2e7076ae0a89def3331218dfa3d&group_state=expanded

(must be on top of bug 1301024)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d18275f35b3d
Part 1: Make CustomElementDefinition ref-counted and put it in CustomElementData. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/05e1f74416aa
Part 2: Get CustomElementDefinition from CustomElementData when possible. r=smaug

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d18275f35b3d
https://hg.mozilla.org/mozilla-central/rev/05e1f74416aa