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

RESOLVED FIXED in mozilla30

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
10 months ago

People

(Reporter: dbuc, Assigned: wchen)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla30
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Assignee: nobody → mrbkap
I'm actively working on this, but the spec has a bunch of problems. I'll add updates here as I get closer.
Blocks: 811542
Created attachment 762414 [details] [diff] [review]
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!!!
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.
Attachment #762414 - Attachment is obsolete: true
(Reporter)

Comment 4

4 years ago
(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?

Updated

4 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
(Reporter)

Updated

4 years ago
Summary: Update document.register to adhere to the current Custom Element spec as of March → Update document.register to adhere to the latest Custom Element spec
Created attachment 820092 [details] [diff] [review]
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.
Attachment #785189 - Attachment is obsolete: true

Updated

4 years ago
Created attachment 8346311 [details] [diff] [review]
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.
Attachment #820092 - Attachment is obsolete: true
Attachment #8346311 - Flags: review?(wchen)

Updated

4 years ago
Assignee: mrbkap → wchen

Comment 7

3 years ago
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.
(Assignee)

Comment 8

3 years ago
Created attachment 8365803 [details] [diff] [review]
Updated document.register

Filled in the gaps, changed how it processed the processing stack to match the spec and added tests.
Attachment #8346311 - Attachment is obsolete: true
Attachment #8346311 - Flags: review?(wchen)
Attachment #8365803 - Flags: review?(mrbkap)
(Assignee)

Comment 9

3 years ago
Created attachment 8365804 [details] [diff] [review]
diff from wip
Blocks: 964211
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 on attachment 8365803 [details] [diff] [review]
Updated document.register

Please see my comments above.
Attachment #8365803 - Flags: review?(mrbkap)
(Assignee)

Comment 12

3 years ago
Created attachment 8377463 [details] [diff] [review]
Updated document.registerElement
Attachment #8365803 - Attachment is obsolete: true
Attachment #8365804 - Attachment is obsolete: true
Attachment #8377463 - Flags: review?(mrbkap)
(Assignee)

Comment 13

3 years ago
Created attachment 8377465 [details] [diff] [review]
v1 diff v2

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

Comment 14

3 years ago
Created attachment 8377466 [details] [diff] [review]
Updated document.registerElement v2

Uploaded wrong patch last time.
Attachment #8377463 - Attachment is obsolete: true
Attachment #8377463 - Flags: review?(mrbkap)
Attachment #8377466 - Flags: review?(mrbkap)
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.
Attachment #8377466 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 16

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/174ec7df74ae
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/174ec7df74ae
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Keywords: dev-doc-needed

Updated

3 years ago
Depends on: 987031

Updated

a year ago
Depends on: 1276240

Updated

10 months ago
See Also: → bug 1294100
You need to log in before you can comment on or make changes to this bug.