Closed Bug 1301024 Opened 5 years ago Closed 4 years ago

Creating an custom element via createElement/createElementNS should call constructor or run upgrade steps

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: edgar, Assigned: jdai)

References

Details

(Whiteboard: dom-ce-m2)

Attachments

(4 files, 13 obsolete files)

1.44 KB, patch
jdai
: review+
Details | Diff | Splinter Review
12.92 KB, patch
jdai
: review+
Details | Diff | Splinter Review
2.56 KB, patch
jdai
: review+
Details | Diff | Splinter Review
30.90 KB, patch
jdai
: review+
Details | Diff | Splinter Review
See
- https://dom.spec.whatwg.org/#dom-document-createelement
- https://dom.spec.whatwg.org/#concept-create-element

e.g.
customElements.define('x-foo', class extends HTMLElement { constructor() { super(); is_constructed = true; });
var element = document.createElement('x-foo');

element.is_constructed should be true.
Attached patch WIP patch, v1 (obsolete) — Splinter Review
Assignee: nobody → echen
Whiteboard: dom-ce-m2
Attached patch WIP patch, v2 (obsolete) — Splinter Review
TODO:
- Creating a built-in should enqueue an upgrade reaction.
- Tests for Xray.
Attachment #8788818 - Attachment is obsolete: true
Blocks: 1331334
This bug will also cover customized built-in element which runs upgrade steps. So update the summary.
Summary: Creating an autonomous custom element via createElement or createElementNS should run constructor → Creating an custom element via createElement/createElementNS should call constructor or run upgrade steps
Depends on: 1299363
Attached patch WIP patch, v3 (obsolete) — Splinter Review
Attachment #8839034 - Attachment is obsolete: true
Comment on attachment 8856843 [details] [diff] [review]
WIP patch, v3

There are something that we can improve, see bug 1299363 comment #78.
Depends on: 1354013
Attached patch WIP patch, v4 (obsolete) — Splinter Review
Attachment #8856843 - Attachment is obsolete: true
Note: We should add test for createElement rethrowing the exact exception that was thrown from the upgrade (bug 1299363 comment 111).
Since Edgar has been parental leaving, I would like to take this bug.
Assignee: echen → jdai
Attached patch wip, v5 (obsolete) — Splinter Review
Rebased patch and revise wpt tests.
Attachment #8883898 - Attachment is obsolete: true
Attached patch wip, v6 (obsolete) — Splinter Review
- Complete create an element steps.
- Introduce synchronous custom elements flag and set default is true.

TODO: address comment #9.
Attachment #8896877 - Attachment is obsolete: true
Per bug 1299363 comment #74 to bug 1299363 comment #80, we should have fixes for the "normal case" and "nearly-normal case".

* The normal case (createElement, with these changes, no define() calls, one arg) is a noticeable slowdown over the "without these changes"  case. That needs to be profiled, understood, and fixed.

* The nearly-normal case (createElement, with the changes, define() call, one arg), is also much slower than what we have now. Needs to be profiled, understood, and fixed: the one-arg case should have no performance penalty, I would think.
Attached patch wip, v7 (obsolete) — Splinter Review
- Fixed webcomponent mochitest failures.

TODO: 
Address comment #13 performance turning.
Attachment #8897793 - Attachment is obsolete: true
This patch implements https://dom.spec.whatwg.org/#dom-document-createelement.
Attachment #8898680 - Attachment is obsolete: true
Attachment #8903087 - Flags: review?(bugs)
This patch implements https://dom.spec.whatwg.org/#concept-create-element.

- Deprecate synchronous custom elements flag and use aFromParser to instead in NS_NewHTMLElement. The reason is synchronous custom elements flag is determined by 3 places:
1) createElement and createElementNS defatult synchronous custom elements flag set[1]. Synchronous custom elements flag is the same as NOT_FROM_PARSER.

2) clone a node synchronous custom elements flag should be unset[2]. Gecko implementation will not go into NS_NewHTMLElement.

3) create an element for a token[3]. Synchronous custom elements flag is determined by will execute script which is not originally created for the HTML fragment parsing algorithm, then let will execute script be true(step 5 and step 7).

Therefore, we can use aFromParser to instead synchronous custom elements flag.

- I added a preference check at NS_NewHTMLElement before doing LookupCustomElementDefinition(). It can help a normal case(comment 13) which is "without these changes" and "with these changes" should not have noticeable slowdown. 

[1] https://dom.spec.whatwg.org/#dom-document-createelement
[2] https://dom.spec.whatwg.org/#concept-node-clone
[3] https://html.spec.whatwg.org/multipage/parsing.html#create-an-element-for-the-token
Attachment #8903096 - Flags: review?(bugs)
Attachment #8903087 - Flags: review?(bugs) → review+
Comment on attachment 8903096 [details] [diff] [review]
Bug 1301024 - Part 2: Implement create an element steps.

># HG changeset patch
># User John Dai <jdai@mozilla.com>
>Bug 1301024 - Part 2: Implement create an element steps. r=smaug
>
>
>diff --git a/dom/base/CustomElementRegistry.cpp b/dom/base/CustomElementRegistry.cpp
>index c015e8a18adf..fac240cce1e0 100644
>--- a/dom/base/CustomElementRegistry.cpp
>+++ b/dom/base/CustomElementRegistry.cpp
>@@ -937,17 +937,17 @@ DoUpgrade(Element* aElement,
>     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>     return;
>   }
> }
> 
> } // anonymous namespace
> 
> // https://html.spec.whatwg.org/multipage/scripting.html#upgrades
>-void
>+/* static */ void
> CustomElementRegistry::Upgrade(Element* aElement,
>                                CustomElementDefinition* aDefinition,
>                                ErrorResult& aRv)
> {
>   aElement->RemoveStates(NS_EVENT_STATE_UNRESOLVED);
> 
>   RefPtr<CustomElementData> data = aElement->GetCustomElementData();
>   MOZ_ASSERT(data, "CustomElementData should exist");
>@@ -975,18 +975,20 @@ CustomElementRegistry::Upgrade(Element* aElement,
>                                                             namespaceURI);
> 
>         LifecycleCallbackArgs args = {
>           nsDependentAtomString(attrName),
>           NullString(),
>           (attrValue.IsEmpty() ? NullString() : attrValue),
>           (namespaceURI.IsEmpty() ? NullString() : namespaceURI)
>         };
>-        EnqueueLifecycleCallback(nsIDocument::eAttributeChanged, aElement,
>-                                 &args, aDefinition);
>+        nsContentUtils::EnqueueLifecycleCallback(aElement->OwnerDoc(),
>+                                                 nsIDocument::eAttributeChanged,
>+                                                 aElement,
>+                                                 &args, aDefinition);
>       }
>     }
>   }
> 
>   // Step 4.
>   // TODO: Bug 1334043 - Implement connected lifecycle callbacks for custom elements
> 
>   // Step 5.
>@@ -1001,17 +1003,19 @@ CustomElementRegistry::Upgrade(Element* aElement,
>     data->mReactionQueue.Clear();
>     return;
>   }
> 
>   // Step 8.
>   data->mState = CustomElementData::State::eCustom;
> 
>   // This is for old spec.
>-  EnqueueLifecycleCallback(nsIDocument::eCreated, aElement, nullptr, aDefinition);
>+  nsContentUtils::EnqueueLifecycleCallback(aElement->OwnerDoc(),
>+                                           nsIDocument::eCreated,
>+                                           aElement, nullptr, aDefinition);
> }
> 
> //-----------------------------------------------------
> // CustomElementReactionsStack
> 
> void
> CustomElementReactionsStack::CreateAndPushElementQueue()
> {
>diff --git a/dom/base/CustomElementRegistry.h b/dom/base/CustomElementRegistry.h
>index 0b5bcb0afbe9..bf8c46448681 100644
>--- a/dom/base/CustomElementRegistry.h
>+++ b/dom/base/CustomElementRegistry.h
>@@ -357,32 +357,33 @@ public:
>   void EnqueueLifecycleCallback(nsIDocument::ElementCallbackType aType,
>                                 Element* aCustomElement,
>                                 LifecycleCallbackArgs* aArgs,
>                                 CustomElementDefinition* aDefinition);
> 
>   void GetCustomPrototype(nsIAtom* aAtom,
>                           JS::MutableHandle<JSObject*> aPrototype);
> 
>+  void SyncInvokeReactions(nsIDocument::ElementCallbackType aType,
>+                           Element* aCustomElement,
>+                           CustomElementDefinition* aDefinition);
>+
>   /**
>    * Upgrade an element.
>    * https://html.spec.whatwg.org/multipage/scripting.html#upgrades
>    */
>-  void Upgrade(Element* aElement, CustomElementDefinition* aDefinition, ErrorResult& aRv);
>+  static void Upgrade(Element* aElement, CustomElementDefinition* aDefinition, ErrorResult& aRv);
> 
> private:
>   ~CustomElementRegistry();
> 
>   UniquePtr<CustomElementCallback> CreateCustomElementCallback(
>     nsIDocument::ElementCallbackType aType, Element* aCustomElement,
>     LifecycleCallbackArgs* aArgs, CustomElementDefinition* aDefinition);
> 
>-  void SyncInvokeReactions(nsIDocument::ElementCallbackType aType,
>-                           Element* aCustomElement,
>-                           CustomElementDefinition* aDefinition);
>   /**
>    * Registers an unresolved custom element that is a candidate for
>    * upgrade when the definition is registered via registerElement.
>    * |aTypeName| is the name of the custom element type, if it is not
>    * provided, then element name is used. |aTypeName| should be provided
>    * when registering a custom element that extends an existing
>    * element. e.g. <button is="x-button">.
>    */
>diff --git a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp
>index 98faa956d68a..c17fc1a8c61e 100644
>--- a/dom/base/nsContentUtils.cpp
>+++ b/dom/base/nsContentUtils.cpp
>@@ -10140,16 +10140,69 @@ nsContentUtils::GetElementDefinitionIfObservingAttr(Element* aCustomElement,
>   if (!definition || !definition->IsInObservedAttributeList(aAttrName)) {
>     return nullptr;
>   }
> 
>   return definition;
> }
> 
> /* static */ void
>+nsContentUtils::SyncInvokeReactions(nsIDocument::ElementCallbackType aType,
>+                                    Element* aElement,
>+                                    CustomElementDefinition* aDefinition)
>+{
>+  MOZ_ASSERT(aElement);
>+
>+  nsCOMPtr<nsIDocument> doc = aElement->OwnerDoc();
>+
>+  if (!doc) {
>+    return;
>+  }
No need to null check doc. OwnerDoc() never ever returns null.
And nsCOMPtr isn't needed here. nsIDocument* is enough


>+
>+  nsCOMPtr<nsPIDOMWindowInner> window(doc->GetInnerWindow());
>+  if (!window) {
>+    return;
>+  }
That could be also just nsPIDOMWindowInner*

>+
>+  RefPtr<CustomElementRegistry> registry(window->CustomElements());
(but here strong reference is probably needed to keep registry alive when SyncInvokeReactions is being called)


>+/* static */ void
>+nsContentUtils::EnqueueUpgradeReaction(Element* aElement,
>+                                       CustomElementDefinition* aDefinition)
>+{
>+  MOZ_ASSERT(aElement);
>+
>+  nsCOMPtr<nsIDocument> doc = aElement->OwnerDoc();
>+
>+  if (!doc) {
>+    return;
>+  }
>+
>+  nsCOMPtr<nsPIDOMWindowInner> window(doc->GetInnerWindow());
>+  if (!window) {
>+    return;
>+  }
Similar things here.


>+
>+  RefPtr<CustomElementRegistry> registry(window->CustomElements());
>+  if (!registry) {
>+    return;
>+  }
>+
>+  CustomElementReactionsStack* stack =
>+    doc->GetDocGroup()->CustomElementReactionsStack();
>+  stack->EnqueueUpgradeReaction(registry, aElement, aDefinition);
>+}
>+
>+/* static */ void
> nsContentUtils::EnqueueLifecycleCallback(nsIDocument* aDoc,
>                                          nsIDocument::ElementCallbackType aType,
>                                          Element* aCustomElement,
>                                          LifecycleCallbackArgs* aArgs,
>                                          CustomElementDefinition* aDefinition)
> {
>   MOZ_ASSERT(aDoc);
> 
>diff --git a/dom/base/nsContentUtils.h b/dom/base/nsContentUtils.h
>index 4165fea0a937..ddf0e2d26af0 100644
>--- a/dom/base/nsContentUtils.h
>+++ b/dom/base/nsContentUtils.h
>@@ -2960,16 +2960,23 @@ public:
>   static void SetupCustomElement(Element* aElement,
>                                  const nsAString* aTypeExtension = nullptr);
> 
>   static mozilla::dom::CustomElementDefinition*
>   GetElementDefinitionIfObservingAttr(Element* aCustomElement,
>                                       nsIAtom* aExtensionType,
>                                       nsIAtom* aAttrName);
> 
>+  static void SyncInvokeReactions(nsIDocument::ElementCallbackType aType,
>+                                  Element* aCustomElement,
>+                                  mozilla::dom::CustomElementDefinition* aDefinition);
>+
>+  static void EnqueueUpgradeReaction(Element* aElement,
>+                                     mozilla::dom::CustomElementDefinition* aDefinition);
>+
>   static void EnqueueLifecycleCallback(nsIDocument* aDoc,
>                                        nsIDocument::ElementCallbackType aType,
>                                        Element* aCustomElement,
>                                        mozilla::dom::LifecycleCallbackArgs* aArgs = nullptr,
>                                        mozilla::dom::CustomElementDefinition* aDefinition = nullptr);
> 
>   static void GetCustomPrototype(nsIDocument* aDoc,
>                                  int32_t aNamespaceID,
>diff --git a/dom/html/nsHTMLContentSink.cpp b/dom/html/nsHTMLContentSink.cpp
>index 275f2f66fcba..996ed3718012 100644
>--- a/dom/html/nsHTMLContentSink.cpp
>+++ b/dom/html/nsHTMLContentSink.cpp
>@@ -219,16 +219,36 @@ public:
>     nsIContent *Add(nsIContent *child);
>   };
> 
>   Node* mStack;
>   int32_t mStackSize;
>   int32_t mStackPos;
> };
> 
>+static void
>+DoCustomElementCreate(Element** aElement,
>+                      CustomElementConstructor* aConstructor, ErrorResult& aRv)
>+{
>+  RefPtr<Element> element =
>+    aConstructor->Construct("Custom Element Create", aRv);
>+  if (aRv.Failed() || !element->IsHTMLElement()) {
>+    aRv.ThrowTypeError<MSG_THIS_DOES_NOT_IMPLEMENT_INTERFACE>(NS_LITERAL_STRING("HTMLElement"));
>+    return;
>+  }
>+
>+  if (element->GetParentNode() || element->HasChildren() ||
>+      element->GetAttrCount()) {
>+    aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
>+    return;
>+  }

This is missing the "If result’s node document is not document, then throw a NotSupportedError. " check.
You probably need to pass the nodeinfo's document here and ensure that element->OwnerDoc() is that.

>+
>+  NS_IF_ADDREF(*aElement = element);
element.forget(aElement);

>+  // https://dom.spec.whatwg.org/#concept-create-element
>+  // We only handle the "synchronous custom elements flag is set" now.
>+  // For the unset case (e.g. cloning a node), file bug 1319342 for that.
s/file bug 1319342 for that/see bug 1319342 for that/


>+  // Step 4.
>+  CustomElementDefinition* definition = nullptr;
>+  if (CustomElementRegistry::IsCustomElementEnabled()) {
>+    definition =
>+      nsContentUtils::LookupCustomElementDefinition(nodeInfo->GetDocument(),
>+                                                    nodeInfo->LocalName(),
>+                                                    nodeInfo->NamespaceID(),
>+                                                    aIs);
>+  }
>+
>+  // It might be a problem that parser synchronously calls constructor, so file
>+  // bug 1378079 to figure out what we should do for parser case.
s/file/filed/ or something

>+  if (aFromParser == NOT_FROM_PARSER && definition) {
Why we get definition always if we end up using it only when aFromParser == NOT_FROM_PARSER

>+    // Step 6.1.
>+    if (aFromParser == NOT_FROM_PARSER) {
This is inside another aFromParser == NOT_FROM_PARSER check.


>+      DoCustomElementCreate(aResult, definition->mConstructor, rv);
>+      if (rv.MaybeSetPendingException(cx)) {
>+        NS_IF_ADDREF(*aResult = NS_NewHTMLUnknownElement(nodeInfo.forget(), aFromParser));
>+      }
>+      return NS_OK;
>+    }
>+
>+    // Step 6.2.
>+    NS_IF_ADDREF(*aResult = NS_NewHTMLElement(nodeInfo.forget(), aFromParser));
>+    nsContentUtils::EnqueueUpgradeReaction(*aResult, definition);
>+    return NS_OK;
So we never execute this, I think. 
Some 'if' around here is wrong.
Attachment #8903096 - Flags: review?(bugs) → review-
Comment on attachment 8903097 [details] [diff] [review]
Bug 1301024 - Part 3: Make the constructor created by document.registerElement() also set CustomElementData and invoke created callback.

>+
>+    // It'll be removed when we deprecate custom elements v0.
>+    nsContentUtils::SyncInvokeReactions(nsIDocument::eCreated, element,
>+                                        definition);
Shouldn't we call this after the WrapNative below?
Explain and ask review again, or fix.

>     NS_ENSURE_TRUE(element, false);
>   }
> 
>   // The prototype setup happens in Element::WrapObject().
>   nsresult rv = nsContentUtils::WrapNative(aCx, element, element, args.rval());
>   NS_ENSURE_SUCCESS(rv, true);
> 
>   return true;
Attachment #8903097 - Flags: review?(bugs) → review-
Attachment #8903098 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #20)
> Comment on attachment 8903096 [details] [diff] [review]
> Bug 1301024 - Part 2: Implement create an element steps.
> 
Thanks for your review.

> >diff --git a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp
> >index 98faa956d68a..c17fc1a8c61e 100644
> >--- a/dom/base/nsContentUtils.cpp
> >+++ b/dom/base/nsContentUtils.cpp
> >@@ -10140,16 +10140,69 @@ nsContentUtils::GetElementDefinitionIfObservingAttr(Element* aCustomElement,
> >   if (!definition || !definition->IsInObservedAttributeList(aAttrName)) {
> >     return nullptr;
> >   }
> > 
> >   return definition;
> > }
> > 
> > /* static */ void
> >+nsContentUtils::SyncInvokeReactions(nsIDocument::ElementCallbackType aType,
> >+                                    Element* aElement,
> >+                                    CustomElementDefinition* aDefinition)
> >+{
> >+  MOZ_ASSERT(aElement);
> >+
> >+  nsCOMPtr<nsIDocument> doc = aElement->OwnerDoc();
> >+
> >+  if (!doc) {
> >+    return;
> >+  }
> No need to null check doc. OwnerDoc() never ever returns null.
> And nsCOMPtr isn't needed here. nsIDocument* is enough
> 
Will do.

> 
> >+
> >+  nsCOMPtr<nsPIDOMWindowInner> window(doc->GetInnerWindow());
> >+  if (!window) {
> >+    return;
> >+  }
> That could be also just nsPIDOMWindowInner*
> 
> >+
> >+  RefPtr<CustomElementRegistry> registry(window->CustomElements());
> (but here strong reference is probably needed to keep registry alive when
> SyncInvokeReactions is being called)
> 
> 
> >+/* static */ void
> >+nsContentUtils::EnqueueUpgradeReaction(Element* aElement,
> >+                                       CustomElementDefinition* aDefinition)
> >+{
> >+  MOZ_ASSERT(aElement);
> >+
> >+  nsCOMPtr<nsIDocument> doc = aElement->OwnerDoc();
> >+
> >+  if (!doc) {
> >+    return;
> >+  }
> >+
> >+  nsCOMPtr<nsPIDOMWindowInner> window(doc->GetInnerWindow());
> >+  if (!window) {
> >+    return;
> >+  }
> Similar things here.
> 
Will do.
> 
> >+
> >+  RefPtr<CustomElementRegistry> registry(window->CustomElements());
> >+  if (!registry) {
> >+    return;
> >+  }
> >+
> >+  CustomElementReactionsStack* stack =
> >+    doc->GetDocGroup()->CustomElementReactionsStack();
> >+  stack->EnqueueUpgradeReaction(registry, aElement, aDefinition);
> >+}
> >+
> >+/* static */ void
> > nsContentUtils::EnqueueLifecycleCallback(nsIDocument* aDoc,
> >                                          nsIDocument::ElementCallbackType aType,
> >                                          Element* aCustomElement,
> >                                          LifecycleCallbackArgs* aArgs,
> >                                          CustomElementDefinition* aDefinition)
> > {
> >   MOZ_ASSERT(aDoc);
> > 
> >diff --git a/dom/base/nsContentUtils.h b/dom/base/nsContentUtils.h
> >index 4165fea0a937..ddf0e2d26af0 100644
> >--- a/dom/base/nsContentUtils.h
> >+++ b/dom/base/nsContentUtils.h
> >@@ -2960,16 +2960,23 @@ public:
> >   static void SetupCustomElement(Element* aElement,
> >                                  const nsAString* aTypeExtension = nullptr);
> > 
> >   static mozilla::dom::CustomElementDefinition*
> >   GetElementDefinitionIfObservingAttr(Element* aCustomElement,
> >                                       nsIAtom* aExtensionType,
> >                                       nsIAtom* aAttrName);
> > 
> >+  static void SyncInvokeReactions(nsIDocument::ElementCallbackType aType,
> >+                                  Element* aCustomElement,
> >+                                  mozilla::dom::CustomElementDefinition* aDefinition);
> >+
> >+  static void EnqueueUpgradeReaction(Element* aElement,
> >+                                     mozilla::dom::CustomElementDefinition* aDefinition);
> >+
> >   static void EnqueueLifecycleCallback(nsIDocument* aDoc,
> >                                        nsIDocument::ElementCallbackType aType,
> >                                        Element* aCustomElement,
> >                                        mozilla::dom::LifecycleCallbackArgs* aArgs = nullptr,
> >                                        mozilla::dom::CustomElementDefinition* aDefinition = nullptr);
> > 
> >   static void GetCustomPrototype(nsIDocument* aDoc,
> >                                  int32_t aNamespaceID,
> >diff --git a/dom/html/nsHTMLContentSink.cpp b/dom/html/nsHTMLContentSink.cpp
> >index 275f2f66fcba..996ed3718012 100644
> >--- a/dom/html/nsHTMLContentSink.cpp
> >+++ b/dom/html/nsHTMLContentSink.cpp
> >@@ -219,16 +219,36 @@ public:
> >     nsIContent *Add(nsIContent *child);
> >   };
> > 
> >   Node* mStack;
> >   int32_t mStackSize;
> >   int32_t mStackPos;
> > };
> > 
> >+static void
> >+DoCustomElementCreate(Element** aElement,
> >+                      CustomElementConstructor* aConstructor, ErrorResult& aRv)
> >+{
> >+  RefPtr<Element> element =
> >+    aConstructor->Construct("Custom Element Create", aRv);
> >+  if (aRv.Failed() || !element->IsHTMLElement()) {
> >+    aRv.ThrowTypeError<MSG_THIS_DOES_NOT_IMPLEMENT_INTERFACE>(NS_LITERAL_STRING("HTMLElement"));
> >+    return;
> >+  }
> >+
> >+  if (element->GetParentNode() || element->HasChildren() ||
> >+      element->GetAttrCount()) {
> >+    aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> >+    return;
> >+  }
> 
> This is missing the "If result’s node document is not document, then throw a
> NotSupportedError. " check.
> You probably need to pass the nodeinfo's document here and ensure that
> element->OwnerDoc() is that.
> 
Will do.

> >+
> >+  NS_IF_ADDREF(*aElement = element);
> element.forget(aElement);
> 
Will do.
> >+  // https://dom.spec.whatwg.org/#concept-create-element
> >+  // We only handle the "synchronous custom elements flag is set" now.
> >+  // For the unset case (e.g. cloning a node), file bug 1319342 for that.
> s/file bug 1319342 for that/see bug 1319342 for that/
> 
Will do.

> 
> >+  // Step 4.
> >+  CustomElementDefinition* definition = nullptr;
> >+  if (CustomElementRegistry::IsCustomElementEnabled()) {
> >+    definition =
> >+      nsContentUtils::LookupCustomElementDefinition(nodeInfo->GetDocument(),
> >+                                                    nodeInfo->LocalName(),
> >+                                                    nodeInfo->NamespaceID(),
> >+                                                    aIs);
> >+  }
> >+
> >+  // It might be a problem that parser synchronously calls constructor, so file
> >+  // bug 1378079 to figure out what we should do for parser case.
> s/file/filed/ or something
> 
Will do.

> >+  if (aFromParser == NOT_FROM_PARSER && definition) {
> Why we get definition always if we end up using it only when aFromParser ==
> NOT_FROM_PARSER
> 
It's because for parser case, we have a |throw-on-dynamic-markup-insertion counter|[1][2] to avoid executing custom element constructors at the same time. However, it'll be implemented at bug 1378079. I guess we can remove |aFromParser == NOT_FROM_PARSER| check in here and add a WPT which execute custom element constructors at the same time should throw an exception at bug 1378079.

[1] https://html.spec.whatwg.org/#throw-on-dynamic-markup-insertion-counter
[2] https://html.spec.whatwg.org/#create-an-element-for-the-token (step 7.)

> >+    // Step 6.1.
> >+    if (aFromParser == NOT_FROM_PARSER) {
> This is inside another aFromParser == NOT_FROM_PARSER check.
> 
We need to keep this check to align spec.
> 
> >+      DoCustomElementCreate(aResult, definition->mConstructor, rv);
> >+      if (rv.MaybeSetPendingException(cx)) {
> >+        NS_IF_ADDREF(*aResult = NS_NewHTMLUnknownElement(nodeInfo.forget(), aFromParser));
> >+      }
> >+      return NS_OK;
> >+    }
> >+
> >+    // Step 6.2.
> >+    NS_IF_ADDREF(*aResult = NS_NewHTMLElement(nodeInfo.forget(), aFromParser));
> >+    nsContentUtils::EnqueueUpgradeReaction(*aResult, definition);
> >+    return NS_OK;
> So we never execute this, I think.
> Some 'if' around here is wrong.
I'll fix in next patch.

(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8903097 [details] [diff] [review]
> Bug 1301024 - Part 3: Make the constructor created by
> document.registerElement() also set CustomElementData and invoke created
> callback.
> 
> >+
> >+    // It'll be removed when we deprecate custom elements v0.
> >+    nsContentUtils::SyncInvokeReactions(nsIDocument::eCreated, element,
> >+                                        definition);
> Shouldn't we call this after the WrapNative below?
> Explain and ask review again, or fix.
If we put SyncInvokeReactions after the WrapNative, it'll break test_custom_element_clone_callbacks_extended.html which shows |logged result after SimpleTest.finish(): Correct prototype should be set on custom elements|. Also, SyncInvokeReactions is for custom element v0, if we put SyncInvokeReactions after the WrapNative, it'll make custom element v1 also file created callback.

> 
> >     NS_ENSURE_TRUE(element, false);
> >   }
> > 
> >   // The prototype setup happens in Element::WrapObject().
> >   nsresult rv = nsContentUtils::WrapNative(aCx, element, element, args.rval());
> >   NS_ENSURE_SUCCESS(rv, true);
> > 
> >   return true;
Blocks: 1396761
(In reply to Edgar Chen [:edgar] (parental leave ~ 10/1; slow response) from comment #9)
> Note: We should add test for createElement rethrowing the exact exception
> that was thrown from the upgrade (bug 1299363 comment 111).

Since this bug block some other bugs, I would like to separate adding a test for createElement rethrowing the exact exception that was thrown from the upgrade to bug 1396761.
- Address comment 20.
- Add a local boolean flag for synchronous custom elements.
Attachment #8903096 - Attachment is obsolete: true
Attachment #8904470 - Flags: review?(bugs)
Comment on attachment 8903097 [details] [diff] [review]
Bug 1301024 - Part 3: Make the constructor created by document.registerElement() also set CustomElementData and invoke created callback.

I would like to ask review again, since I replied it at comment 22.
Attachment #8903097 - Flags: review- → review?(bugs)
Attachment #8903097 - Flags: review?(bugs) → review+
Comment on attachment 8904470 [details] [diff] [review]
Bug 1301024 - Part 2: Implement create an element steps. v2

>+nsContentUtils::SyncInvokeReactions(nsIDocument::ElementCallbackType aType,
>+                                    Element* aElement,
>+                                    CustomElementDefinition* aDefinition)
>+{
>+  MOZ_ASSERT(aElement);
>+
>+  nsCOMPtr<nsIDocument> doc = aElement->OwnerDoc();
Nit, this can be nsIDocument* safely. A bit faster that way.

>+nsContentUtils::EnqueueUpgradeReaction(Element* aElement,
>+                                       CustomElementDefinition* aDefinition)
>+{
>+  MOZ_ASSERT(aElement);
>+
>+  nsCOMPtr<nsIDocument> doc = aElement->OwnerDoc();
this too

>+  // It might be a problem that parser synchronously calls constructor, so filed
>+  // bug 1378079 to figure out what we should do for parser case.
>+  if (definition) {
>+    bool synchronousCustomElements = aFromParser != dom::FROM_PARSER_FRAGMENT;
Could you add a comment here that only setting inner/outerHTML should be non-synchronous 

>+    // Per discussion in https://github.com/w3c/webcomponents/issues/635,
>+    // use entry global in those places that are called from JS APIs.
>+    nsIGlobalObject* global = GetEntryGlobal();
If this method is used to create a custom element from C++, we may not have entry global, so this would crash.
But since creating custom elements in C++ doesn't make much sense, at least now, perhaps ok to just add
MOZ_ASSERT(global);

>+    // Step 6.1.
>+    if (synchronousCustomElements) {
>+      DoCustomElementCreate(aResult, nodeInfo->GetDocument(),
>+                            definition->mConstructor, rv);
>+      if (rv.MaybeSetPendingException(cx)) {
>+        NS_IF_ADDREF(*aResult = NS_NewHTMLUnknownElement(nodeInfo.forget(), aFromParser));
If DoCustomElementCreate throws an exception, aResult may still be set to some value and you end up leaking that.
Perhaps NS_IF_RELEASE(*aResult); before NS_IF_ADDREF call.
Attachment #8904470 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #26)
> Comment on attachment 8904470 [details] [diff] [review]
> Bug 1301024 - Part 2: Implement create an element steps. v2
> 
Thanks for your review.
> >+nsContentUtils::SyncInvokeReactions(nsIDocument::ElementCallbackType aType,
> >+                                    Element* aElement,
> >+                                    CustomElementDefinition* aDefinition)
> >+{
> >+  MOZ_ASSERT(aElement);
> >+
> >+  nsCOMPtr<nsIDocument> doc = aElement->OwnerDoc();
> Nit, this can be nsIDocument* safely. A bit faster that way.
> 
Will do.

> >+nsContentUtils::EnqueueUpgradeReaction(Element* aElement,
> >+                                       CustomElementDefinition* aDefinition)
> >+{
> >+  MOZ_ASSERT(aElement);
> >+
> >+  nsCOMPtr<nsIDocument> doc = aElement->OwnerDoc();
> this too
> 
Will do.

> >+  // It might be a problem that parser synchronously calls constructor, so filed
> >+  // bug 1378079 to figure out what we should do for parser case.
> >+  if (definition) {
> >+    bool synchronousCustomElements = aFromParser != dom::FROM_PARSER_FRAGMENT;
> Could you add a comment here that only setting inner/outerHTML should be
> non-synchronous 
> 
Will do.

> >+    // Per discussion in https://github.com/w3c/webcomponents/issues/635,
> >+    // use entry global in those places that are called from JS APIs.
> >+    nsIGlobalObject* global = GetEntryGlobal();
> If this method is used to create a custom element from C++, we may not have
> entry global, so this would crash.
> But since creating custom elements in C++ doesn't make much sense, at least
> now, perhaps ok to just add
> MOZ_ASSERT(global);
> 
Will do.

> >+    // Step 6.1.
> >+    if (synchronousCustomElements) {
> >+      DoCustomElementCreate(aResult, nodeInfo->GetDocument(),
> >+                            definition->mConstructor, rv);
> >+      if (rv.MaybeSetPendingException(cx)) {
> >+        NS_IF_ADDREF(*aResult = NS_NewHTMLUnknownElement(nodeInfo.forget(), aFromParser));
> If DoCustomElementCreate throws an exception, aResult may still be set to
> some value and you end up leaking that.
> Perhaps NS_IF_RELEASE(*aResult); before NS_IF_ADDREF call.
Will do. I'll also add report exception when DoCustomElementCreate throws an exception.
oops, you don't need to do that NS_IF_RELEASE, since DoCustomElementCreate sets aElement to non-null only when the method succeeds.
Priority: -- → P2
Rebase after firefox 57.
Attachment #8903087 - Attachment is obsolete: true
Attachment #8903097 - Attachment is obsolete: true
Attachment #8903098 - Attachment is obsolete: true
Attachment #8904940 - Attachment is obsolete: true
Attachment #8911698 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/861b7c93c156
Part 1: Set CreateElement/CreateElementNS is attribute. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6fdd6eae583
Part 2: Implement create an element steps. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a47aacfccdb2
Part 3: Make the constructor created by document.registerElement() also set CustomElementData and invoke created callback. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d14b96108c7
Part 4: Update wpt and mochitest tests. r=smaug
Keywords: checkin-needed
Depends on: 1407669
Depends on: 1411088
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.