1406325 - Custom element clone node without definition should put into candidate map in order to upgrade later

Status: RESOLVED FIXED

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

Description

[John Dai[:jdai]]

In our implementation, when custom element clone node without definition should put into candidate map in order to upgrade later. After doing this, we can add back test_custom_element_clone_callbacks.html[1] which was deleted by bug 1319342.

[1] https://hg.mozilla.org/mozilla-central/diff/3bc98b219437/dom/tests/mochitest/webcomponents/test_custom_element_clone_callbacks.html

Comment 1

[Edgar Chen [:edgar]]

Hmm, It seems the cloned node should not be upgraded until append to the document per spec [1] and it is how chrome behaves.
Test script: https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5427

[1] See step 7.7.2.2 of https://dom.spec.whatwg.org/#concept-node-insert

Comment 2

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #1)
> Hmm, It seems the cloned node should not be upgraded until append to the
> document per spec [1] and it is how chrome behaves.
> Test script:
> https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5427
> 
> [1] See step 7.7.2.2 of https://dom.spec.whatwg.org/#concept-node-insert

Yes, you are right. It's because we miss implement try to upgrade[1] in custom element v1.

[1] https://html.spec.whatwg.org/multipage/custom-elements.html#concept-try-upgrade

Comment 3

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 1: Make sure custom element state is custom then send callback.

Per spec[1][2][3], we should fire custom elements callback when custom elements state is custom. In this patch, I add custom state check in connected, disconnected and attribute change callbacks.

[1] https://dom.spec.whatwg.org/#concept-node-insert
[2] https://dom.spec.whatwg.org/#concept-node-remove
[3] https://dom.spec.whatwg.org/#concept-element-attributes-change

Comment 4

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 2: Fix missing CustomElementData setup when cloning a node. v1

Comment 5

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 3: Implement try to upgrade. v1

Per spec[1], we need to implement try to upgrade in order to make unresolved element become a custom element when there is a custom element define.

[1] https://html.spec.whatwg.org/multipage/custom-elements.html#concept-try-upgrade

Comment 6

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 4: Refactor custom elements clone a node.

Try is base on bug 1408828.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bb905449e04f9f172af011e20cf1b55c5158309&filter-tier=1&group_state=expanded

Comment 7

[John Dai[:jdai]]

Comment on attachment 8920497 [details] [diff] [review]
Bug 1406325 - Part 1: Make sure custom element state is custom then send callback.

Changed feedback to Jessica since these code was added by her.

Comment 8

[Edgar Chen [:edgar]]

Comment on attachment 8920497 [details] [diff] [review]
Bug 1406325 - Part 1: Make sure custom element state is custom then send callback.

Review of attachment 8920497 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Element.cpp
@@ +1664,2 @@
>        nsContentUtils::EnqueueLifecycleCallback(nsIDocument::eConnected, this);
> +    }

Does this cause any build error?

@@ +2675,2 @@
>        if (CustomElementDefinition* definition =
> +        nsContentUtils::GetElementDefinitionIfObservingAttr(this,

nsContentUtils::GetElementDefinitionIfObservingAttr() seems already ensure element is in "custom" state, but in an indirect way: CustomElementDefinition currently set to Element only if the state in CustomElementData is eCustom, so in non-custom case, nsContentUtils::GetElementDefinitionIfObservingAttr() will just return nullptr.

Comment 9

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #8)
> Comment on attachment 8920497 [details] [diff] [review]
> Bug 1406325 - Part 1: Make sure custom element state is custom then send
> callback.
> 
> Review of attachment 8920497 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/Element.cpp
> @@ +1664,2 @@
> >        nsContentUtils::EnqueueLifecycleCallback(nsIDocument::eConnected, this);
> > +    }
> 
> Does this cause any build error?
> 
It was accidentally forgot to remove when I was splitting patches.

> @@ +2675,2 @@
> >        if (CustomElementDefinition* definition =
> > +        nsContentUtils::GetElementDefinitionIfObservingAttr(this,
> 
> nsContentUtils::GetElementDefinitionIfObservingAttr() seems already ensure
> element is in "custom" state, but in an indirect way:
> CustomElementDefinition currently set to Element only if the state in
> CustomElementData is eCustom, so in non-custom case,
> nsContentUtils::GetElementDefinitionIfObservingAttr() will just return
> nullptr.
Yes, maybe we can use MOZ_ASSERT in here.

Comment 10

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 1: Make sure custom element state is custom before sending callback. v2

Since Edgar inputed some feedbacks, I'll set this patch feedback to Edgar.

Comment 11

[Edgar Chen [:edgar]]

Comment on attachment 8920502 [details] [diff] [review]
Bug 1406325 - Part 3: Implement try to upgrade. v1

Review of attachment 8920502 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Element.cpp
@@ +1663,5 @@
>      if (data && data->mState == CustomElementData::State::eCustom) {
>        nsContentUtils::EnqueueLifecycleCallback(nsIDocument::eConnected, this);
> +    } else {
> +      // Step 7.7.2.2 https://dom.spec.whatwg.org/#concept-node-insert
> +      nsContentUtils::TryToUpgradeElement(this);

We have to be careful with processing the mCandidatesMap in CustomElementRegistry, if we append an element and then remove it immediately, the element should not be upgraded, right? And I think it worth to have a test for this case.
And if we append the element again, mCandidatesMap will have duplicated element, will this cause any problem? Do we expect to have duplicated element in mCandidatesMap?

::: dom/html/nsHTMLContentSink.cpp
@@ +356,5 @@
>  
>    if (CustomElementRegistry::IsCustomElementEnabled() &&
>        (isCustomElementName || aIs)) {
> +    (*aResult)->SetCustomElementData(new CustomElementData(typeAtom));
> +    if ((*aResult)->IsInComposedDoc()) {

It seems IsInComposedDoc() will never return true at this point (during element creation). The 'IsInDocument' and 'NODE_IS_IN_SHADOW_TREE' flags are mostly set in BindToTree() which usually happens later.

Comment 12

[Edgar Chen [:edgar]]

Comment on attachment 8920520 [details] [diff] [review]
Bug 1406325 - Part 1: Make sure custom element state is custom before sending callback. v2

Review of attachment 8920520 [details] [diff] [review]:
-----------------------------------------------------------------

f+ with comments addressed.

::: dom/base/Element.cpp
@@ +2670,5 @@
>  
>    if (CustomElementRegistry::IsCustomElementEnabled()) {
>      if (CustomElementData* data = GetCustomElementData()) {
>        if (CustomElementDefinition* definition =
> +        nsContentUtils::GetElementDefinitionIfObservingAttr(this,

I noticed the change is only for the indentation, could you revert it if it is an oversight?

@@ +2976,5 @@
>    }
>  
>    if (CustomElementRegistry::IsCustomElementEnabled()) {
> +    CustomElementData* data = GetCustomElementData();
> +    if (data && data->mState == CustomElementData::State::eCustom) {

Just like comment #8, nsContentUtils::GetElementDefinitionIfObservingAttr() already ensures the element is in "custom" state. (Sorry for didn't point out in previous review).

Comment 13

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 1: Make sure custom element state is custom before sending callback. v3

Address comment 12.

Comment 14

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 3: Implement try to upgrade. v2

Address comment 11.

Comment 15

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 5: Implement element should have an associated is value. v1

Per spec[1], is value should keep in element and its value is given when create an element[2].

[1] https://dom.spec.whatwg.org/#concept-element-is-value
[2] https://dom.spec.whatwg.org/#concept-create-element

Comment 16

[John Dai[:jdai]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b56dd7f0eeb4f018c32a8685359a7b9693f883e9&filter-tier=1&group_state=expanded

Comment 17

[Edgar Chen [:edgar]]

Comment on attachment 8924082 [details] [diff] [review]
Bug 1406325 - Part 3: Implement try to upgrade. v2

Review of attachment 8924082 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/CustomElementRegistry.cpp
@@ +327,5 @@
> +{
> +  nsTArray<nsWeakPtr>* candidates;
> +  if (mCandidatesMap.Get(aTypeName, &candidates)) {
> +    MOZ_ASSERT(candidates);
> +    for (size_t i = 0; i < candidates->Length(); ++i) {

We iterate the candidates array and remove the element from the array whenever an unresolved custom element is removed from tree. It looks like a bit expensive to me. It would be good if we could think about "is it possible to do something different to improve the performance", but this could be done on a separated bug, IMO.

::: dom/base/nsContentUtils.cpp
@@ +10122,5 @@
> +{
> +  NodeInfo* nodeInfo = aElement->NodeInfo();
> +  RefPtr<nsAtom> tagAtom = nodeInfo->NameAtom();
> +  nsAutoString extension;
> +  aElement->GetAttr(kNameSpaceID_None, nsGkAtoms::is, extension);

Part 5 modifies this to use 'is' value stored in CustomElementData. It seems we could consider switching the patch order: make this part depending on the part that adds `is` value under CustomElementData, then we could direct use the 'is' value stored in CustomElementData in this part, instead of modifying again in subsequent part.

@@ +10221,5 @@
> +  RefPtr<nsAtom> tagAtom = nodeInfo->NameAtom();
> +  nsAutoString extension;
> +  aElement->GetAttr(kNameSpaceID_None, nsGkAtoms::is, extension);
> +  RefPtr<nsAtom> typeAtom = extension.IsEmpty() ? tagAtom : NS_Atomize(extension);
> +  if (nsContentUtils::IsCustomElementName(tagAtom) || !extension.IsEmpty()) {

It seems to me that we don't need to check is-element-a-custom-elemet here, since the caller only pass the aElement which has CustomElementData and only an custom element could have CustomElementData, so we already ensure the aElement is an custom element.

We could probably add assertion to ensure the element should have CustomElementData and also not in custom state maybe.

Comment 18

[Edgar Chen [:edgar]]

Comment on attachment 8924082 [details] [diff] [review]
Bug 1406325 - Part 3: Implement try to upgrade. v2

Review of attachment 8924082 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/nsHTMLContentSink.cpp
@@ -368,5 @@
>    }
>  
>    if (CustomElementRegistry::IsCustomElementEnabled() &&
>        (isCustomElementName || aIs)) {
> -    nsContentUtils::SetupCustomElement(*aResult, aIs);

This is the last bit that use nsContentUtils::SetupCustomElement(), it's time to remove nsContentUtils::SetupCustomElement(). :)

Comment 19

[Edgar Chen [:edgar]]

Comment on attachment 8924083 [details] [diff] [review]
Bug 1406325 - Part 5: Implement element should have an associated is value. v1

Review of attachment 8924083 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/CustomElementRegistry.cpp
@@ +46,5 @@
>          // fail to find the CE definition for this custom element.
>          // This will go away eventually since there is no created callback in v1.
>          CustomElementDefinition* definition =
>            nsContentUtils::LookupCustomElementDefinition(document,
> +            ni->LocalName(), ni->NamespaceID(), typeAtom);

It seems to me that we do some necessary conversion, we could just use `mOwnerData->mType`, though this piece of codes are going to be removed in bug 1396620.

::: dom/base/CustomElementRegistry.h
@@ +131,5 @@
>    // There are 3 reactions in reaction queue when doing upgrade operation,
>    // e.g., create an element, insert a node.
>    AutoTArray<UniquePtr<CustomElementReaction>, 3> mReactionQueue;
>  
> +  RefPtr<nsAtom> mIs;

Add some comments to explain what possible value for built-in element or anonymous custom element, something like http://searchfox.org/mozilla-central/rev/423b2522c48e1d654e30ffc337164d677f934ec3/dom/base/CustomElementRegistry.h#117-118.

::: dom/base/Element.h
@@ +574,5 @@
>     * @param aDefinition The custom element definition.
>     */
>    void SetCustomElementDefinition(CustomElementDefinition* aDefinition);
>  
> +  nsAtom* GetCustomElementIsValue() const;

Add comments.

::: dom/html/nsHTMLContentSink.cpp
@@ +370,5 @@
>  
>    if (CustomElementRegistry::IsCustomElementEnabled() &&
>        (isCustomElementName || aIs)) {
>      (*aResult)->SetCustomElementData(new CustomElementData(typeAtom));
> +    (*aResult)->SetCustomElementIsValue(typeAtom);

For <x-foo>, we set is-value to "x-foo", this doesn't look right.

Comment 20

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #17)
> Comment on attachment 8924082 [details] [diff] [review]
> Bug 1406325 - Part 3: Implement try to upgrade. v2
> 
> Review of attachment 8924082 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/CustomElementRegistry.cpp
> @@ +327,5 @@
> > +{
> > +  nsTArray<nsWeakPtr>* candidates;
> > +  if (mCandidatesMap.Get(aTypeName, &candidates)) {
> > +    MOZ_ASSERT(candidates);
> > +    for (size_t i = 0; i < candidates->Length(); ++i) {
> 
> We iterate the candidates array and remove the element from the array
> whenever an unresolved custom element is removed from tree. It looks like a
> bit expensive to me. It would be good if we could think about "is it
> possible to do something different to improve the performance", but this
> could be done on a separated bug, IMO.
> 
We can use a dirty bit for an unresolved custom element, hence, it's no need to iterate the candidates array and remove the element from the array.


(In reply to Edgar Chen [:edgar] from comment #19)
> Comment on attachment 8924083 [details] [diff] [review]
> Bug 1406325 - Part 5: Implement element should have an associated is value.
> v1
> 
> Review of attachment 8924083 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/CustomElementRegistry.cpp
> @@ +46,5 @@
> >          // fail to find the CE definition for this custom element.
> >          // This will go away eventually since there is no created callback in v1.
> >          CustomElementDefinition* definition =
> >            nsContentUtils::LookupCustomElementDefinition(document,
> > +            ni->LocalName(), ni->NamespaceID(), typeAtom);
> 
> It seems to me that we do some necessary conversion, we could just use
> `mOwnerData->mType`, though this piece of codes are going to be removed in
> bug 1396620.
> 
It would be nice if we can use mType instead of introduce a is value, because we only need is value for LookupCustomElementDefinition.

Comment 21

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #17)
> Comment on attachment 8924082 [details] [diff] [review]
> Bug 1406325 - Part 3: Implement try to upgrade. v2
> 
> Review of attachment 8924082 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/CustomElementRegistry.cpp
> @@ +327,5 @@
> > +{
> > +  nsTArray<nsWeakPtr>* candidates;
> > +  if (mCandidatesMap.Get(aTypeName, &candidates)) {
> > +    MOZ_ASSERT(candidates);
> > +    for (size_t i = 0; i < candidates->Length(); ++i) {
> 
> We iterate the candidates array and remove the element from the array
> whenever an unresolved custom element is removed from tree. It looks like a
> bit expensive to me. It would be good if we could think about "is it
> possible to do something different to improve the performance", but this
> could be done on a separated bug, IMO.
> 
File bug 1414178 for tracking. Not sure it's a performance cliff for custom element, we need more data before doing this performance enhancement.

Comment 22

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 3: Refactor custom elements clone a node.

Move this part to part3 and carry f+.

Comment 23

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 4: Implement element should have an associated is value.

- Move part5 to part4.
- Address comment 18 and comment 19.
- Use mType in custom element data to instead mIs, it's because we only need is value for LookupCustomElementDefinition and it use mType to lookup definition.
- Revise mType to private data.

Comment 24

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 5: Implement try to upgrade.

- Address comment 17 which is reorder patches.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5ccab02292dde148872d85bfee7edbdf892f63b&filter-tier=1&group_state=expanded

Comment 25

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 5: Implement try to upgrade.

Upload the correct patch.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=44ad5882af743d9aa818eb45df6bcd54031e68bb&filter-tier=1&group_state=expanded

Comment 26

[Edgar Chen [:edgar]]

Comment on attachment 8924870 [details] [diff] [review]
Bug 1406325 - Part 4: Implement element should have an associated is value.

Review of attachment 8924870 [details] [diff] [review]:
-----------------------------------------------------------------

> Bug 1406325 - Part 4: Implement element should have an associated is value. r=smaug

We don't actually implement 'is value' in this version, but reuse the mType in CustomElementData, please revise the commit message.

Comment 27

[Edgar Chen [:edgar]]

Comment on attachment 8924874 [details] [diff] [review]
Bug 1406325 - Part 5: Implement try to upgrade.

Review of attachment 8924874 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/CustomElementRegistry.cpp
@@ +329,5 @@
> +{
> +  nsTArray<nsWeakPtr>* candidates;
> +  if (mCandidatesMap.Get(aTypeName, &candidates)) {
> +    MOZ_ASSERT(candidates);
> +    for (size_t i = 0; i < candidates->Length(); ++i) {

Add comment with the follow-up bug number.

Comment 28

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 1: Make sure custom element state is custom before sending callback. f=echen, r=smaug

Comment 29

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 2: Set CustomElementData when cloning a node. f=echen, r=smaug

Comment 30

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 3: Refactor custom elements clone a node. f=echen, r=smaug

Comment 31

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 4: Use mType for LookupCustomElementDefinition. f=echen, r=smaug

Comment 32

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 5: Implement try to upgrade. f=echen, r=smaug

Comment 33

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

Comment on attachment 8924906 [details] [diff] [review]
Bug 1406325 - Part 3: Refactor custom elements clone a node. f=echen, r=smaug

># HG changeset patch
># User John Dai <[e-mail]>
>Bug 1406325 - Part 3: Refactor custom elements clone a node. f=echen, r=smaug
>
>
>diff --git a/dom/base/nsNodeUtils.cpp b/dom/base/nsNodeUtils.cpp
>index f2e43b9ceb86..73ad22b758a1 100644
>--- a/dom/base/nsNodeUtils.cpp
>+++ b/dom/base/nsNodeUtils.cpp
>@@ -462,44 +462,32 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNode, bool aClone, bool aDeep,
>       aError.Throw(rv);
>       return nullptr;
>     }
> 
>     if (CustomElementRegistry::IsCustomElementEnabled() && clone->IsElement()) {
>       // The cloned node may be a custom element that may require
>       // enqueing upgrade reaction.
>       Element* elem = clone->AsElement();
>-      CustomElementDefinition* definition = nullptr;
>       RefPtr<nsAtom> tagAtom = nodeInfo->NameAtom();
>-      if (nsContentUtils::IsCustomElementName(tagAtom)) {
>-        elem->SetCustomElementData(new CustomElementData(tagAtom));
>-        definition =
>+      // Check if node may be custom element by type extension.
>+      // ex. <button is="x-button">
>+      nsAutoString extension;
>+      elem->GetAttr(kNameSpaceID_None, nsGkAtoms::is, extension);
So we now always call GetAttr

>+      RefPtr<nsAtom> typeAtom = extension.IsEmpty() ? tagAtom : NS_Atomize(extension);
>+      if (nsContentUtils::IsCustomElementName(tagAtom) || !extension.IsEmpty()) {
...and check IsCustomElementName()
that looks slow(er), and we should make things faster.

Comment 34

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

Comment on attachment 8924907 [details] [diff] [review]
Bug 1406325 - Part 4: Use mType for LookupCustomElementDefinition. f=echen, r=smaug


>   if (CustomElementRegistry::IsCustomElementEnabled() &&
>       (isCustomElementName || aIs)) {
>-    nsContentUtils::SetupCustomElement(*aResult, aIs);
>+    (*aResult)->SetCustomElementData(new CustomElementData(typeAtom));
I'm a bit lost now. nsContentUtils::SetupCustomElement does end up calling
SyncInvokeReactions(nsIDocument::eCreated, aElement, definition);
I tried and failed to see how this is now handled.
I'm probably just missing something. Please explain and ask review again.

Comment 35

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

ahaa, perhaps the next patch explains some of this

Comment 36

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

Or not.

Comment 37

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

Comment on attachment 8924908 [details] [diff] [review]
Bug 1406325 - Part 5: Implement try to upgrade. f=echen, r=smaug

>+CustomElementRegistry::UnregisterUnresolvedElement(Element* aElement,
>+                                                   nsAtom* aTypeName)
>+{
>+  nsTArray<nsWeakPtr>* candidates;
>+  if (mCandidatesMap.Get(aTypeName, &candidates)) {
>+    MOZ_ASSERT(candidates);
>+    // We don't need to iterate the candidates array and remove the element from
>+    // the array for performance reason. It'll be handle by bug 1396620.
>+    for (size_t i = 0; i < candidates->Length(); ++i) {
>+      nsCOMPtr<Element> elem = do_QueryReferent(candidates->ElementAt(i));
>+      if (elem && elem.get() == aElement) {
>+        candidates->RemoveElementAt(i);
>+      }
>+    }
>+  }
>+}
>+




>+  /**
>+   * Unregist an unresolved custom element that is a candidate for
Unregister

>@@ -2061,19 +2066,23 @@ Element::UnbindFromTree(bool aDeep, bool aNullParent)
>     }
> 
>     document->ClearBoxObjectFor(this);
> 
>      // Disconnected must be enqueued whenever a connected custom element becomes
>      // disconnected.
>     if (CustomElementRegistry::IsCustomElementEnabled()) {
>       CustomElementData* data  = GetCustomElementData();
>-      if (data && data->mState == CustomElementData::State::eCustom) {
>-        nsContentUtils::EnqueueLifecycleCallback(nsIDocument::eDisconnected,
>-                                                 this);
>+      if (data) {
>+        if (data->mState == CustomElementData::State::eCustom) {
>+          nsContentUtils::EnqueueLifecycleCallback(nsIDocument::eDisconnected,
>+                                                   this);
>+        } else {
>+          nsContentUtils::UnregisterUnresolvedElement(this);
What in the spec says this should happen? Explain (and ask review again) or fix.

Comment 38

[Edgar Chen [:edgar]]

(In reply to Olli Pettay [:smaug] from comment #33)
> Comment on attachment 8924906 [details] [diff] [review]
> Bug 1406325 - Part 3: Refactor custom elements clone a node. f=echen, r=smaug
> >+      nsAutoString extension;
> >+      elem->GetAttr(kNameSpaceID_None, nsGkAtoms::is, extension);
> So we now always call GetAttr
> 
> >+      RefPtr<nsAtom> typeAtom = extension.IsEmpty() ? tagAtom : NS_Atomize(extension);
> >+      if (nsContentUtils::IsCustomElementName(tagAtom) || !extension.IsEmpty()) {
> ...and check IsCustomElementName()
> that looks slow(er), and we should make things faster.

Yeah, we probably could decide whether to create CustomElementData by checking the original node. Put it this way, if we clone a node which is a custom element, the cloned element should also a custom element, then we could just check whether the original node has CustomElementData. Does this make sense?

Comment 39

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #38) 
> Yeah, we probably could decide whether to create CustomElementData by
> checking the original node. Put it this way, if we clone a node which is a
> custom element, the cloned element should also a custom element, then we
> could just check whether the original node has CustomElementData. Does this
> make sense?

On a second thought, depending on CustomElementData of original node could probably be wrong for some cases, especially for custom built-in element.

For example, if we clear `is` attribute before clone the element, but the CustomElementData in original element is created for original 'is' attribute, in this case, using CustomElementData from the original node could be wrong ...

Comment 40

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #38)
> (In reply to Olli Pettay [:smaug] from comment #33)
> > Comment on attachment 8924906 [details] [diff] [review]
> > Bug 1406325 - Part 3: Refactor custom elements clone a node. f=echen, r=smaug
> > >+      nsAutoString extension;
> > >+      elem->GetAttr(kNameSpaceID_None, nsGkAtoms::is, extension);
> > So we now always call GetAttr
> > 
> > >+      RefPtr<nsAtom> typeAtom = extension.IsEmpty() ? tagAtom : NS_Atomize(extension);
> > >+      if (nsContentUtils::IsCustomElementName(tagAtom) || !extension.IsEmpty()) {
> > ...and check IsCustomElementName()
> > that looks slow(er), and we should make things faster.
> 
> Yeah, we probably could decide whether to create CustomElementData by
> checking the original node. Put it this way, if we clone a node which is a
> custom element, the cloned element should also a custom element, then we
> could just check whether the original node has CustomElementData. Does this
> make sense?

It's a good idea to instead nsContentUtils::IsCustomElementName. However, it only apply for autonomous custom element. Per spec[1], it should get is attribute if present, consider a case, we have a custom element which has is value, and we change its is vaule after custom element is created. When cloning a node, it should apply new is value. If we use original node's CustomElementData, it'll apply a CustomElementData which contains old is value. There are four cases for an original node:
1) Original node has CustomElementData and it's an autonomous custom element. 
2) Original node has CustomElementData and it's a built-in custom element.
3) Original node doesn't have CustomElementData and it's an autonomous custom element. There is no this case.
4) Original node doesn't have CustomElementData and it appends an is value after node created.

If there is a CustomElementData, it'll imply that the original node is an autonomous custom element or a built-in custom element. We don't need to do anything for autonomous custom element, but we need to get is value for built-in custom element and case 4).


[1] https://dom.spec.whatwg.org/#concept-node-clone (step 2.1)

Comment 41

[John Dai[:jdai]]

(In reply to Olli Pettay [:smaug] from comment #34)
> Comment on attachment 8924907 [details] [diff] [review]
> Bug 1406325 - Part 4: Use mType for LookupCustomElementDefinition. f=echen,
> r=smaug
> 
> 
> >   if (CustomElementRegistry::IsCustomElementEnabled() &&
> >       (isCustomElementName || aIs)) {
> >-    nsContentUtils::SetupCustomElement(*aResult, aIs);
> >+    (*aResult)->SetCustomElementData(new CustomElementData(typeAtom));
> I'm a bit lost now. nsContentUtils::SetupCustomElement does end up calling
> SyncInvokeReactions(nsIDocument::eCreated, aElement, definition);
> I tried and failed to see how this is now handled.
> I'm probably just missing something. Please explain and ask review again.

SyncInvokeReactions will be deprecated by bug 1396620. Per spec[1], there is no createdCallback in custom element v1. I'll ask review again. Thank you.

[1] https://html.spec.whatwg.org/multipage/custom-elements.html#concept-custom-element-definition-lifecycle-callbacks

Comment 42

[John Dai[:jdai]]

> >@@ -2061,19 +2066,23 @@ Element::UnbindFromTree(bool aDeep, bool aNullParent)
> >     }
> > 
> >     document->ClearBoxObjectFor(this);
> > 
> >      // Disconnected must be enqueued whenever a connected custom element becomes
> >      // disconnected.
> >     if (CustomElementRegistry::IsCustomElementEnabled()) {
> >       CustomElementData* data  = GetCustomElementData();
> >-      if (data && data->mState == CustomElementData::State::eCustom) {
> >-        nsContentUtils::EnqueueLifecycleCallback(nsIDocument::eDisconnected,
> >-                                                 this);
> >+      if (data) {
> >+        if (data->mState == CustomElementData::State::eCustom) {
> >+          nsContentUtils::EnqueueLifecycleCallback(nsIDocument::eDisconnected,
> >+                                                   this);
> >+        } else {
> >+          nsContentUtils::UnregisterUnresolvedElement(this);
> What in the spec says this should happen? Explain (and ask review again) or
> fix.

It's because we didn't put all elements that are shadow-including descendants of document in upgrade candidates[1][2] (see bug 1326028). Hence, we need to put an element in upgrade candidates when we do try to upgrade and remove it in upgrade candidates when we remove this element from tree. It would be better if I put more comments on it.

[1] https://html.spec.whatwg.org/multipage/custom-elements.html#element-definition (Step 14)
[2] https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/dom/base/CustomElementRegistry.cpp#517-542

Comment 43

[John Dai[:jdai]]

(In reply to John Dai[:jdai] from comment #42)
> > >@@ -2061,19 +2066,23 @@ Element::UnbindFromTree(bool aDeep, bool aNullParent)
> > >     }
> > > 
> > >     document->ClearBoxObjectFor(this);
> > > 
> > >      // Disconnected must be enqueued whenever a connected custom element becomes
> > >      // disconnected.
> > >     if (CustomElementRegistry::IsCustomElementEnabled()) {
> > >       CustomElementData* data  = GetCustomElementData();
> > >-      if (data && data->mState == CustomElementData::State::eCustom) {
> > >-        nsContentUtils::EnqueueLifecycleCallback(nsIDocument::eDisconnected,
> > >-                                                 this);
> > >+      if (data) {
> > >+        if (data->mState == CustomElementData::State::eCustom) {
> > >+          nsContentUtils::EnqueueLifecycleCallback(nsIDocument::eDisconnected,
> > >+                                                   this);
> > >+        } else {
> > >+          nsContentUtils::UnregisterUnresolvedElement(this);
> > What in the spec says this should happen? Explain (and ask review again) or
> > fix.
> 
> It's because we didn't put all elements that are shadow-including
> descendants of document in upgrade candidates[1][2] (see bug 1326028).
> Hence, we need to put an element in upgrade candidates when we do try to
> upgrade and remove it in upgrade candidates when we remove this element from
> tree. It would be better if I put more comments on it.
> 
> [1]
> https://html.spec.whatwg.org/multipage/custom-elements.html#element-
> definition (Step 14)
> [2]
> https://searchfox.org/mozilla-central/rev/
> bab833ebeef6b2202e71f81d225b968283521fd6/dom/base/CustomElementRegistry.
> cpp#517-542

I would like to explain more clearly. We maintain an upgrade candidates map when a unresolved custom element is put into tree and is removed from tree, but it's not shadow-including tree order. In bug 1326028, we will make sure all unresolved custom elements should be shadow-including tree order in upgrade candidates map.

Comment 44

[Edgar Chen [:edgar]]

(In reply to Olli Pettay [:smaug] from comment #37)
> >@@ -2061,19 +2066,23 @@ Element::UnbindFromTree(bool aDeep, bool aNullParent)
> >     }
> > 
> >     document->ClearBoxObjectFor(this);
> > 
> >      // Disconnected must be enqueued whenever a connected custom element becomes
> >      // disconnected.
> >     if (CustomElementRegistry::IsCustomElementEnabled()) {
> >       CustomElementData* data  = GetCustomElementData();
> >-      if (data && data->mState == CustomElementData::State::eCustom) {
> >-        nsContentUtils::EnqueueLifecycleCallback(nsIDocument::eDisconnected,
> >-                                                 this);
> >+      if (data) {
> >+        if (data->mState == CustomElementData::State::eCustom) {
> >+          nsContentUtils::EnqueueLifecycleCallback(nsIDocument::eDisconnected,
> >+                                                   this);
> >+        } else {
> >+          nsContentUtils::UnregisterUnresolvedElement(this);
> What in the spec says this should happen? Explain (and ask review again) or
> fix.

In addition to comment #42 and comment #43, I would like to explain more,

Spec says "when new definition arrives, we should enqueue upgrade reactions for all the elements that are shadow-including descendants of document, whose local name or is value matches with the definition" [1]. Currently, we add element to mCandidatesMap[2] at the element creation time[3] (if the element is a custom element and definition is not registered yet).
But this actually doesn't match with the spec, i.e. for the element that is not connected to the document, it is not "shadow-including descendants of document", and it should not be upgraded per spec, but we still upgrade it.

So in this part, we change the way of maintaining mCandidatesMap to match the spec,
- Put the element to mCandidatesMap when it is connected to the document.
- And Remove it from mCandidatesMap when it is disconnected.

[1] step 14 and step 15 of https://html.spec.whatwg.org/multipage/custom-elements.html#element-definition
[2] https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/dom/base/CustomElementRegistry.h#454
[3] https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/dom/html/nsHTMLContentSink.cpp#372

Comment 45

[Edgar Chen [:edgar]]

(In reply to Olli Pettay [:smaug] from comment #34)
> Comment on attachment 8924907 [details] [diff] [review]
> Bug 1406325 - Part 4: Use mType for LookupCustomElementDefinition. f=echen,
> r=smaug
> 
> 
> >   if (CustomElementRegistry::IsCustomElementEnabled() &&
> >       (isCustomElementName || aIs)) {
> >-    nsContentUtils::SetupCustomElement(*aResult, aIs);
> >+    (*aResult)->SetCustomElementData(new CustomElementData(typeAtom));
> I'm a bit lost now. nsContentUtils::SetupCustomElement does end up calling
> SyncInvokeReactions(nsIDocument::eCreated, aElement, definition);
> I tried and failed to see how this is now handled.
> I'm probably just missing something. Please explain and ask review again.

In addition to comment #41, I would like to explain more.

In fact, we always early return at [1] in current usage, since the only place calling SetupCustomElement is [2], which is for the "element is a custom element and definition is not yet registered" case.

Given that SetupCustomElement only does following things in current usage,
1) Set `is` attribute if the element is a built-in custom element and has no 'is' attribute.
2) Setup CustomElementData
3) Register element to mCandidatesMap

For 1), it is for the document.createElement[3] initially, but we already handle it in [4] after bug 1301024.
For 3), we change the way of maintaining mCandidatesMap in next part (see comment #44).
So the only thing that [2] needs is 2). 

[1] https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/dom/base/CustomElementRegistry.cpp#352
[2] https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/dom/html/nsHTMLContentSink.cpp#372
[3] Step 6 of https://dom.spec.whatwg.org/#dom-document-createelement
[4] https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/dom/base/nsDocument.cpp#6117

Comment 46

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

ok, "when new definition arrives, we should enqueue upgrade reactions for all the elements that are shadow-including descendants of document, whose local name or is value matches with the definition" explains it. Thanks. Could you add some comment to the code.

Comment 47

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

(In reply to John Dai[:jdai] from comment #41)
> (In reply to Olli Pettay [:smaug] from comment #34)
> > Comment on attachment 8924907 [details] [diff] [review]
> > Bug 1406325 - Part 4: Use mType for LookupCustomElementDefinition. f=echen,
> > r=smaug
> > 
> > 
> > >   if (CustomElementRegistry::IsCustomElementEnabled() &&
> > >       (isCustomElementName || aIs)) {
> > >-    nsContentUtils::SetupCustomElement(*aResult, aIs);
> > >+    (*aResult)->SetCustomElementData(new CustomElementData(typeAtom));
> > I'm a bit lost now. nsContentUtils::SetupCustomElement does end up calling
> > SyncInvokeReactions(nsIDocument::eCreated, aElement, definition);
> > I tried and failed to see how this is now handled.
> > I'm probably just missing something. Please explain and ask review again.
> 
> SyncInvokeReactions will be deprecated by bug 1396620. Per spec[1], there is
> no createdCallback in custom element v1. I'll ask review again. Thank you.
> 

I see, we're doing several things in this bug. What the bug title says and also removing parts of v0. That is fine, just a bit confusing. 
Thanks for explanation.

Comment 48

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 3: Refactor custom elements clone a node. f=echen, r=smaug

Address comment 40. 

- Use old CustomElementData to instead nsContentUtils::IsCustomElementName.
- Get is value when original node has CustomElementData and it's a built-in custom element and original node doesn't have CustomElementData and it appends an is value after node created.

Comment 49

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 4: Use mType for LookupCustomElementDefinition and also removing parts of v0. f=echen, r=smaug

Revise the commit message.

Comment 50

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 5: Implement try to upgrade. f=echen, r=smaug

Add more comment to the code.

Comment 51

[John Dai[:jdai]]

Attachment: Bug 1406325 - Part 3: Refactor custom elements clone a node. f=echen, r=smaug

Revise using clone element to get is value.

Comment 52

[John Dai[:jdai]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f298f2fa2a15299f935ebe73256e33d863bc7f6&filter-tier=1&group_state=expanded

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf10bc890feb
Part 1: Make sure custom element state is custom before sending callback. f=echen, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ed19ef50fe6
Part 2: Set CustomElementData when cloning a node. f=echen, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/625ef8fa4bd3
Part 3: Refactor custom elements clone a node. f=echen, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/af93cbd1ebfd
Part 4: Use mType for LookupCustomElementDefinition and also removing parts of v0. f=echen, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/067aabd5d35d
Part 5: Implement try to upgrade. f=echen, r=smaug

Comment 54

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/cf10bc890feb
https://hg.mozilla.org/mozilla-central/rev/3ed19ef50fe6
https://hg.mozilla.org/mozilla-central/rev/625ef8fa4bd3
https://hg.mozilla.org/mozilla-central/rev/af93cbd1ebfd
https://hg.mozilla.org/mozilla-central/rev/067aabd5d35d