Last Comment Bug 856140 - Update document.register to adhere to the latest Custom Element spec
: Update document.register to adhere to the latest Custom Element spec
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
-- normal with 1 vote (vote)
: mozilla30
Assigned To: William Chen [:wchen]
:
: Andrew Overholt [:overholt]
Mentors:
http://w3c.github.io/webcomponents/sp...
Depends on: 987031 1276240
Blocks: webcomponents 964211
  Show dependency treegraph
 
Reported: 2013-03-29 12:59 PDT by Daniel Buchner [:dbuc]
Modified: 2016-08-10 10:18 PDT (History)
16 users (show)
wchen: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (38.29 KB, patch)
2013-06-13 17:30 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Updated to trunk. Still a wip. (39.10 KB, patch)
2013-08-02 13:59 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
wip, getting closer (41.52 KB, patch)
2013-10-21 19:14 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
wip (47.93 KB, patch)
2013-12-11 18:43 PST, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Updated document.register (87.87 KB, patch)
2014-01-26 23:29 PST, William Chen [:wchen]
no flags Details | Diff | Splinter Review
diff from wip (62.89 KB, patch)
2014-01-26 23:30 PST, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Updated document.registerElement (87.87 KB, patch)
2014-02-18 02:03 PST, William Chen [:wchen]
no flags Details | Diff | Splinter Review
v1 diff v2 (75.11 KB, patch)
2014-02-18 02:04 PST, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Updated document.registerElement v2 (117.55 KB, patch)
2014-02-18 02:07 PST, William Chen [:wchen]
mrbkap: review+
Details | Diff | Splinter Review

Description User image Daniel Buchner [:dbuc] 2013-03-29 12:59:29 PDT
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 User image Blake Kaplan (:mrbkap) 2013-05-17 15:38:10 PDT
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 User image Blake Kaplan (:mrbkap) 2013-06-13 17:30:53 PDT
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!!!
Comment 3 User image Blake Kaplan (:mrbkap) 2013-08-02 13:59:19 PDT
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.
Comment 4 User image Daniel Buchner [:dbuc] 2013-08-16 08:02:53 PDT
(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 User image Blake Kaplan (:mrbkap) 2013-10-21 19:14:43 PDT
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.
Comment 6 User image Blake Kaplan (:mrbkap) 2013-12-11 18:43:21 PST
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.
Comment 7 User image robdodson 2014-01-21 16:08:24 PST
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 User image William Chen [:wchen] 2014-01-26 23:29:59 PST
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.
Comment 9 User image William Chen [:wchen] 2014-01-26 23:30:16 PST
Created attachment 8365804 [details] [diff] [review]
diff from wip
Comment 10 User image Blake Kaplan (:mrbkap) 2014-02-10 10:00:19 PST
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 User image Blake Kaplan (:mrbkap) 2014-02-10 10:00:44 PST
Comment on attachment 8365803 [details] [diff] [review]
Updated document.register

Please see my comments above.
Comment 12 User image William Chen [:wchen] 2014-02-18 02:03:08 PST
Created attachment 8377463 [details] [diff] [review]
Updated document.registerElement
Comment 13 User image William Chen [:wchen] 2014-02-18 02:04:17 PST
Created attachment 8377465 [details] [diff] [review]
v1 diff v2

Addressed review comments.
Renamed types to be more consistent with the spec.
Added extension.
Comment 14 User image William Chen [:wchen] 2014-02-18 02:07:03 PST
Created attachment 8377466 [details] [diff] [review]
Updated document.registerElement v2

Uploaded wrong patch last time.
Comment 15 User image Blake Kaplan (:mrbkap) 2014-02-18 13:05:37 PST
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 17 User image Carsten Book [:Tomcat] 2014-02-24 04:21:32 PST
https://hg.mozilla.org/mozilla-central/rev/174ec7df74ae

Note You need to log in before you can comment on or make changes to this bug.