856140 - Update document.register to adhere to the latest Custom Element spec

Status: RESOLVED FIXED

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

Description

[Daniel Buchner [:dbuc]]

The shake out of moving lifecycle callbacks to the prototype and the attach/detach methods should not affect the end-user operation of the tags people will create with our sugar layer. I'd like to update our implementation to match.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

I'm actively working on this, but the spec has a bunch of problems. I'll add updates here as I get closer.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: wip

Still to do:
* Move all of this stuff to the right "browsing context" instead of the document.
* Fix all of the XXXs (lots of error handling stuff).
* Understand how the "override existing name" stuff is supposed to work.
* Test. Test. Test!!!

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to trunk. Still a wip.

This patch applies to current trunk. There's still a bunch of XXX
comments to fix, but it's ironically closer to the spec now due to spec
changes over the past month.

Comment 4

[Daniel Buchner [:dbuc]]

(In reply to Blake Kaplan (:mrbkap) from comment #3)
> Created attachment 785189 [details] [diff] [review]
> Updated to trunk. Still a wip.
> 
> This patch applies to current trunk. There's still a bunch of XXX
> comments to fix, but it's ironically closer to the spec now due to spec
> changes over the past month.

Given the move to hold off on the declarative API, how far do you think we are from having the implementation to-date for the imperative version?

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: wip, getting closer

Still to do: implement the lifecycle callbacks (the spec isn't a great match for our codebase, so it'll be a little awkward) and, based on that, the element upgrade algorithm. Also, need to implement is attribute.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: wip

wchen is going to take this patch over the finish line. Here's a list of stuff I know needs to be done:

* Add the concept of the "microtask checkpoint" before all events. We should share this with nsXPConnect::OnProcessNextEvent. We should then call (static) nsDocument::ProcessBaseElementQueue from it (and disentagle that from nsDocument::ProcessTopElementQueue).

* Call the lifetime callbacks from Element::BindToTree, Element::SetAttr.

* Fix the XXX comments scattered around.

Comment 7

[robdodson]

We noticed if an element is used after it is registered it is never upgraded. Here's an example: https://gist.github.com/azakus/8550960

In this case you'll get a "created a" message, but no "created b". It seems like once document.register/registerElement is called then all tags should be upgraded.

Comment 8

[William Chen [:wchen]]

Attachment: Updated document.register

Filled in the gaps, changed how it processed the processing stack to match the spec and added tests.

Comment 9

[William Chen [:wchen]]

Attachment: diff from wip

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8365804 [details] [diff] [review]
diff from wip

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

I have one comment that I'd like to see addressed before checkin, but this looks really good overall.

::: content/base/public/nsIDocument.h
@@ +1999,5 @@
> +  virtual nsresult RegisterUnresolvedElement(Element *aElement) = 0;
> +  virtual void EnqueueLifecycleCallback(ElementCallbackType aType,
> +                 Element* aCustomElement,
> +                 mozilla::dom::LifecycleCallbackArgs* aArgs = nullptr,
> +                 mozilla::dom::CustomElementData* aData = nullptr) = 0;

Nit: the second through fourth arguments should line up with the first one.

::: content/base/src/nsDocument.cpp
@@ +5453,5 @@
>    if (!data) {
>      nsINodeInfo* info = aCustomElement->NodeInfo();
>      CustomElementHashKey key(info->NamespaceID(), info->NameAtom());
>      if (!mRegistry->mCustomPrototypes.Get(&key, &data)) {
>        NS_WARNING("Enqueuing callback for non-custom-element?");

Won't we hit this from BindToTree for elements that have valid custom element names but aren't actually custom elements?

@@ +5497,5 @@
> +  CustomElementCallbackData* callbackData;
> +  sCallbackDataMap.ref().Get(aCustomElement, &callbackData);
> +
> +  if (!callbackData) {
> +    callbackData = new CustomElementCallbackData();

I wonder if we're ever going to want to just stick this on the DOM object slots...

@@ -5690,5 @@
>  
>        JS::RootedObject wrapper(aCx);
>        if ((wrapper = cache->GetWrapper())) {
>          if (!JS_SetPrototype(aCx, wrapper, protoObject)) {
> -          // XXX XXX XXX ????

One thing I was expecting to see somewhere that I'm not seeing here is the first step at <http://w3c.github.io/webcomponents/spec/custom/#dfn-custom-element-constructor-generation> (I'd put it up where we do the other prototype stuff because otherwise it happens too late).

::: content/base/src/nsDocument.h
@@ +287,5 @@
>    int32_t mNamespaceID;
>    nsCOMPtr<nsIAtom> mAtom;
>  };
>  
> +struct LifecycleCallbackArgs {

Nit (here and below): I believe the New Style Guide (TM) specifies that the opening braces of structs go on the following line.

@@ +304,5 @@
>    nsRefPtr<mozilla::dom::Element> mThisObject;
>    nsRefPtr<mozilla::dom::CallbackFunction> mCallback;
> +  nsIDocument::ElementCallbackType mType;
> +  LifecycleCallbackArgs mArgs;
> +  CustomElementCallbackData* mOwnerData;

Now that there's a constructor here, can the members be private with only Call as a public function?

@@ +312,5 @@
> +// Each custom element has an associated callback queue and an element is
> +// being created flag.
> +struct CustomElementCallbackData {
> +  CustomElementCallbackData();
> +  nsTArray<nsAutoPtr<CustomElementCallback> > mCallbackQueue;

I think we can do away with the space after the > now:

nsTArray<nsAutoPtr<...>> mCallbackQueue;

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8365803 [details] [diff] [review]
Updated document.register

Please see my comments above.

Comment 12

[William Chen [:wchen]]

Attachment: Updated document.registerElement

Comment 13

[William Chen [:wchen]]

Attachment: v1 diff v2

Addressed review comments.
Renamed types to be more consistent with the spec.
Added extension.

Comment 14

[William Chen [:wchen]]

Attachment: Updated document.registerElement v2

Uploaded wrong patch last time.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8377466 [details] [diff] [review]
Updated document.registerElement v2

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

One nit. Let's fix it and land this thing.

::: content/base/src/nsDocument.cpp
@@ +1995,5 @@
>      nsHostObjectProtocolHandler::Traverse(tmp->mHostObjectURIs[i], cb);
>    }
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>  
> +struct CustomDefinitionTraceArgs {

Nit: { on its own line.

Comment 16

[William Chen [:wchen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/174ec7df74ae

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/174ec7df74ae