Last Comment Bug 783129 - Implement the document.register interface method
: Implement the document.register interface method
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla22
Assigned To: William Chen [:wchen]
:
Mentors:
http://dvcs.w3.org/hg/webcomponents/r...
: 824151 (view as bug list)
Depends on: 820544 820957 828532 849866
Blocks: webcomponents 824151
  Show dependency treegraph
 
Reported: 2012-08-15 16:50 PDT by Daniel Buchner [:dbuc]
Modified: 2014-02-21 09:44 PST (History)
30 users (show)
wchen: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP of document.register implementation (20.76 KB, patch)
2012-09-04 07:03 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Implementation of document.register without shadow DOM support. v2 (20.48 KB, patch)
2012-10-15 14:46 PDT, William Chen [:wchen]
mrbkap: review+
bent.mozilla: review+
Details | Diff | Splinter Review
v1 diff v2 (9.04 KB, patch)
2012-10-15 14:46 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Implementation of document.register without shadow DOM support. v3 (35.48 KB, patch)
2012-12-06 11:57 PST, William Chen [:wchen]
no flags Details | Diff | Splinter Review
v2 diff v3 (11.28 KB, patch)
2012-12-06 11:57 PST, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Implementation of document.register without shadow DOM support. v4 (36.85 KB, patch)
2012-12-06 16:54 PST, William Chen [:wchen]
mrbkap: review+
Details | Diff | Splinter Review
WebIDL version of implementation. (34.28 KB, patch)
2012-12-12 10:39 PST, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Patch that hacks around the orange (3.24 KB, patch)
2012-12-21 16:20 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Implementation of document.register without shadow DOM support. v5 (41.53 KB, patch)
2013-02-25 22:14 PST, William Chen [:wchen]
no flags Details | Diff | Splinter Review
interdiff (19.54 KB, patch)
2013-02-25 22:15 PST, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Implementation of document.register without shadow DOM support. v6 (41.92 KB, patch)
2013-02-26 11:31 PST, William Chen [:wchen]
mrbkap: review+
Details | Diff | Splinter Review
v5 diff v6 (6.83 KB, patch)
2013-02-26 11:31 PST, William Chen [:wchen]
no flags Details | Diff | Splinter Review

Description Daniel Buchner [:dbuc] 2012-08-15 16:50:57 PDT
This piece of the Web Components spec is at a point where (IMO) implementation can begin on the first foundational step: bringing custom element recognition to the DOM. The document.register method is the base of custom elements and does not rely on any other section of the Web Components family of technologies - it basically provides 50% of the benefit of the total possible hotness of Web Components without having to implement all the others parts of the spec just yet (Shadow DOM, Templates, etc).

The relevant spec doc can be found here: http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/custom/index.html#extensions-to-document-interface

If there are any implementation issues or questions on direction/intent, please ping me and I can arrange a discussion with Dimitri Glazkov from the Webkit team at Google.
Comment 1 Olli Pettay [:smaug] 2012-08-15 16:58:46 PDT
Well, the template part needs support for Shadow DOM.
Comment 2 Daniel Buchner [:dbuc] 2012-08-15 17:04:03 PDT
Given that adding a Template is already an optional input, I wonder if we could make document.register fully independent from it? Perhaps Dimitri can chime in to let us know.
Comment 3 Dimitri Glazkov 2012-08-15 20:43:21 PDT
(In reply to Daniel Buchner [:dbuc] from comment #2)
> Given that adding a Template is already an optional input, I wonder if we
> could make document.register fully independent from it? Perhaps Dimitri can
> chime in to let us know.

Yes, the template is spec'd as optional specifically based on Daniel's input. He managed to build a pretty convincing set of custom DOM elements without shadow DOM in x-tags.

I am totally fine starting with an implementation without template support. It's perfectly additive, which means we can add it later without jeopardizing the cross-platform support.
Comment 4 Olli Pettay [:smaug] 2012-08-16 02:18:37 PDT
The draft isn't clear what should happen when "element upgrade algorithm" runs.
What is the DOM tree there? Something which has Document as root? Or any DOM tree, which may have
DocumentFragment or Element?
The result of the algorithm is anyway that we may end up having x-myelements with different prototypes
and behaviors (if someone keeps a reference to the original element before replace).

It is not defined where elementupgrade is dispatched.

Is it possible to create custom elements with document.createElement()?
What about .innerHTML ?

...but ok, these questions should go to webapps wg.
Comment 5 Olli Pettay [:smaug] 2012-08-16 02:21:40 PDT
The draft isn't clear what should happen when "element upgrade algorithm" runs.
What is the DOM tree there? Something which has Document as root? Or any DOM tree, which may have
DocumentFragment or Element? Replace doesn't work without parent, so if the element is the root...
If the idea is to replace only elements in Document rooted tree we may end up having x-myelements with different 
prototypes and behaviors (if someone keeps a reference to the original element before replace).

It is not defined where elementupgrade is dispatched.

Is it possible to create custom elements with document.createElement()?
What about .innerHTML ?

...ok, these questions should go to WebApps WG.
Comment 6 Daniel Buchner [:dbuc] 2012-08-16 07:33:27 PDT
We've specifically addressed many of these issues in our spec meetings, and I'd like to give Dimitri a chance to address the details you've brought up - though I can tell you the lifecycle phase where elements are enhanced occurs wherever inflation takes place, innerHTML, createElement, source parse, etc.

I'd rather sort the nth detail out here instead of punting to the working group abyss that would later require a submersible with a hull rating of "Marianas Trench" to retrieve...
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2012-08-16 07:56:10 PDT
(In reply to Daniel Buchner [:dbuc] from comment #6)
> I'd rather sort the nth detail out here instead of punting to the working
> group abyss that would later require a submersible with a hull rating of
> "Marianas Trench" to retrieve...

Your attitude to standardization bothers me. Are you implementing anything web-exposed?
Comment 8 Olli Pettay [:smaug] 2012-08-16 08:01:27 PDT
A browser vendor bug is not the right place to discuss about (somewhat major) spec issues.

In this case WebApps WG mailing list and/or W3C bugzilla is the right place.
Comment 9 Daniel Buchner [:dbuc] 2012-08-16 08:34:37 PDT
Beep beep back the truck up folks. I was asking that you allow Dimitri to step in and walk you through the process here. Much of what has been raised as issues frankly aren't because we've addressed them. If we can all take a second to understand the finer points of the proposal, I am confident that we may find the answers are there.

As far as my attitude toward standardization: I have been attending these Web Components standard spec meetings for about 3 months now. Dimitiri and the rest of the folks at Google have taken all our input seriously and incorporated many of the ideas we brought to the table. Additionally, we have a Web Components-focused project in-house that will be our organizational presentation of an advanced, future-forward, standards-leaning method for constructing apps for Firefox OS and Desktop apps - this effort is product-sensitive, and as a result, support for standards that would advance it should be punted back as a last resort.

(I personally feel 12 months of spec work and active participation by three vendors is a solid foundation for implementation discussion, but I digress)
Comment 10 Dimitri Glazkov 2012-08-16 09:22:27 PDT
Hi :dbuc, :Ms2ger, and :smaug!

(In reply to Olli Pettay [:smaug] from comment #5)
> The draft isn't clear what should happen when "element upgrade algorithm"
> runs.
> What is the DOM tree there? Something which has Document as root? Or any DOM
> tree, which may have
> DocumentFragment or Element? Replace doesn't work without parent, so if the
> element is the root...

Yes! This is https://www.w3.org/Bugs/Public/show_bug.cgi?id=18535. I'll give it a go today.

> If the idea is to replace only elements in Document rooted tree we may end
> up having x-myelements with different 
> prototypes and behaviors (if someone keeps a reference to the original
> element before replace).

The last bit where someone keeps a reference to the original element is the reason why we do the replaceChild -- so that even if someone keeps a reference to non-upgraded element, we don't have (or have to invent a mechanism for) any behavior changing from under the reference-holder's feet. One idea to mitigate the confusion between original and upgraded elements is https://www.w3.org/Bugs/Public/show_bug.cgi?id=18585.
 
> It is not defined where elementupgrade is dispatched.

Good catch: https://www.w3.org/Bugs/Public/show_bug.cgi?id=18594

> 
> Is it possible to create custom elements with document.createElement()?

Not at the moment.

> What about .innerHTML ?

Yes. I need to add more informative content to section 5 to enumerate the effects of hooking into creating element for a token step of the parser: https://www.w3.org/Bugs/Public/show_bug.cgi?id=18595

> 
> ...ok, these questions should go to WebApps WG.

Yup. Let's continue spec discussion on either WG bugzilla or mailing lists, and implementation discussion here. Happy to help with either one.
Comment 11 Daniel Buchner [:dbuc] 2012-08-16 09:33:55 PDT
In reference to the "when do you know components are swapped in" issue, we've discussed the possibility of a ready event, DOMComponentsLoaded. I introduced this event in X-Tag after a few developers asked for it, it fires when all components present in static source are strapped and ready to be accessed by user code.
Comment 12 William Chen [:wchen] 2012-08-29 10:54:16 PDT
I think we need to talk more about how we handle element upgrade (or maybe I have missed the discussion). Right now the spec says to do a node replace, however this means we will lose our attributes and children on the nodes already in the tree. This makes it feel more like a element reset rather than an upgrade. Would it make sense to try and transplant some important properties on the node like the attributes and children?
Comment 13 Dimitri Glazkov 2012-08-29 11:01:23 PDT
Yes, will fix this shortly: https://www.w3.org/Bugs/Public/show_bug.cgi?id=18732

My thinking is that we transplant attributes and children only, not event listeners.
Comment 14 Daniel Buchner [:dbuc] 2012-08-29 11:08:42 PDT
(In reply to Dimitri Glazkov from comment #13)
> Yes, will fix this shortly:
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=18732
> 
> My thinking is that we transplant attributes and children only, not event
> listeners.

What's your thinking on how folks can go about baking default event listeners into custom elements? Is the upgrade event able to be defined on the prototype so that listeners can be added to all upgraded nodes of a certain type without a separate code block from the custom element definition?
Comment 15 Dimitri Glazkov 2012-08-29 11:26:13 PDT
Can we move discussion to spec bug? :) I don't want to lose valuable questions/insight from you folks.
Comment 16 William Chen [:wchen] 2012-09-04 07:03:55 PDT
Created attachment 658066 [details] [diff] [review]
WIP of document.register implementation

Here is a WIP that implements what we can without support for templates and shadow DOM.
Comment 17 Blake Kaplan (:mrbkap) 2012-09-24 19:38:14 PDT
Comment on attachment 658066 [details] [diff] [review]
WIP of document.register implementation

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

I have a couple of drive-by comments here. This looks really good though... I wonder if we might be able to land it if we throw NOT_IMPLEMENTED if someone passes us a shadow tree.

::: content/base/src/nsDocument.cpp
@@ +1579,5 @@
>  
> +  nsISupports* supports;
> +  QueryInterface(NS_GET_IID(nsCycleCollectionISupports), reinterpret_cast<void**>(&supports));
> +  NS_ASSERTION(supports, "Failed to QI to nsCycleCollectionISupports?!");
> +  nsContentUtils::DropJSObjects(supports);

Is there any reason this can't use NS_DROP_JS_OBJECTS(this, nsDocument)?

@@ +2051,5 @@
> +
> +  nsISupports* thisSupports;
> +  QueryInterface(NS_GET_IID(nsCycleCollectionISupports), reinterpret_cast<void**>(&thisSupports));
> +  NS_ASSERTION(thisSupports, "Failed to QI to nsCycleCollectionISupports!");
> +  rv = nsContentUtils::HoldJSObjects(thisSupports, participant);

And similarly, NS_HOLD_JS_OBJECTS here?

@@ +4639,5 @@
> +  JSObject* callee = JSVAL_TO_OBJECT(JS_CALLEE(aCx, aVp));
> +
> +  nsIXPConnect* xpc = nsContentUtils::XPConnect();
> +
> +  jsval documentVal = js::GetFunctionNativeReserved(callee, 0);

Instead of using a reserved slot, you can simply use the function object's compartment's global (which will be a window) to get at the document. That'll simplify this a little, I think.

@@ +4646,5 @@
> +  nsCOMPtr<nsIXPConnectWrappedNative> wrappedNative;
> +  xpc->GetWrappedNativeOfJSObject(aCx, documentObj,
> +                                  getter_AddRefs(wrappedNative));
> +  nsCOMPtr<nsIDOMDocument> document =
> +      do_QueryInterface(wrappedNative->Native());

This'll change if you make the above change, but for the record, this could be do_QueryWrappedNative.

@@ +4666,5 @@
> +  JSObject* global = JS_GetGlobalForObject(aCx, callee);
> +
> +  jsval v;
> +  rv = nsContentUtils::WrapNative(aCx, global, newElement,
> +                                  (nsWrapperCache*) nullptr, &v);

Why do you need the cast?

@@ +4715,5 @@
> +  jsval customTemplate = JSVAL_VOID;
> +
> +  // Get optional arguments if provided.
> +  if (aArgc > 0) {
> +    NS_ENSURE_TRUE(!JSVAL_IS_PRIMITIVE(aOptions), NS_ERROR_UNEXPECTED);

You should probably use the HTML5 dictionary stuff to extract these values.

@@ +4741,5 @@
> +                                     &protoObject);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  } else {
> +    NS_ENSURE_TRUE(!JSVAL_IS_PRIMITIVE(customProto) &&
> +                   !JSVAL_IS_NULL(customProto),

Nit: JSVAL_IS_NULL is subsumed by the check for JSVAL_IS_PRIMITIVE. That being said, with a bit of extra work, you could also use the member functions on JS::Value, which are a little clearer and The Future (TM).

@@ +4756,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    // Check the proto chain for HTMLElement prototype.
> +    JSObject* protoProto = JS_GetPrototype(protoObject);
> +    bool isInChain = false;

Nit: isInChain isn't used.

@@ +4775,5 @@
> +  // Do element upgrade.
> +  nsRefPtr<nsContentList> list = GetElementsByTagName(lcName);
> +  for (int32_t i = 0; i < list->Length(false); i++) {
> +    nsINode* currentNode = list->Item(i, false);
> +    nsCOMPtr<nsIDOMNode> oldNode = do_QueryInterface(currentNode);

C++ code (especially inside content/!) shouldn't use nsIDOMNode. Instead, use the functions available on nsINode.

@@ +4782,5 @@
> +    }
> +
> +    // TODO(wchen): Hook up to templates when avilable.
> +    nsCOMPtr<nsIDOMNode> newNode;
> +    rv = oldNode->CloneNode(true, 1, getter_AddRefs(newNode));

So this would use nsINode::Clone.

@@ +4827,5 @@
> +  // Put the document in the first reserved slot.
> +  js::SetFunctionNativeReserved(constructorObject, 0, OBJECT_TO_JSVAL(GetWrapper()));
> +
> +  // Put element name in the second reserved slot.
> +  JSString* elemName = JS_NewUCStringCopyZ(aCx, lcName.get());

Does the spec allow us to stick this info in the name of the function? If so, we can do away with the reserved slots, which would be nice.

::: dom/base/nsDOMClassInfo.cpp
@@ +8256,5 @@
> +  Element *element = static_cast<Element*>(wrapper->Native());
> +  nsAutoString elementName;
> +  nsContentUtils::ASCIIToLower(element->NodeName(), elementName);
> +  if (StringBeginsWith(elementName, NS_LITERAL_STRING("x-"))) {
> +    return PostCreate(wrapper, cx, obj);

Is this right? If I take an x-tag node and adopt it into another document, shouldn't it keep the same prototype?
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2012-09-25 00:50:51 PDT
Huh, why are we doing anything with x-tags? We decided we wouldn't do that.
Comment 19 Blake Kaplan (:mrbkap) 2012-09-25 06:40:59 PDT
This is document.register, which is a related API but bound to Shadow DOM. When I said x-tags I meant a "registered element."
Comment 20 :Ms2ger (⌚ UTC+1/+2) 2012-09-25 07:25:54 PDT
But then I saw

if (StringBeginsWith(elementName, NS_LITERAL_STRING("x-"))) {
Comment 21 Blake Kaplan (:mrbkap) 2012-09-26 07:01:42 PDT
(In reply to :Ms2ger from comment #20)
> if (StringBeginsWith(elementName, NS_LITERAL_STRING("x-"))) {

The spec requires that all registered elements' names begin with "x-".
Comment 22 Daniel Buchner [:dbuc] 2012-09-26 09:53:40 PDT
(In reply to :Ms2ger from comment #20)
> But then I saw
> 
> if (StringBeginsWith(elementName, NS_LITERAL_STRING("x-"))) {

Just to clarify:

1. X-Tag is the JS polylib that provides the same basic functionality document.register custom elements do.

2. The spec originally said the element name would be specified through an is="" attribute. Being that the attribute cannot be changed (changing it would not have any effect), it is a poor choice as it creates an implicit expectation of  mutability among downstream developer-users.

3. Shadow DOM and Templates are in the Web Components family, but Custom Elements does not need those APIs to be a huge win for developers (I'd argue, the biggest win of them all) - thus the API was specified in a way that the others APIs in the Web Components family are icing on the cake.

Does this help answer some of the direction/specification questions Ms2ger? Any other questions/concerns?
Comment 23 :Ms2ger (⌚ UTC+1/+2) 2012-09-26 10:00:04 PDT
My main question is this: will document.createElement("x-foo") or <x-foo> in markup be the expected way to use your own foo-thing? If so, we don't want to do that because the fallback and a11y story sucks.
Comment 24 William Chen [:wchen] 2012-09-26 10:14:41 PDT
(In reply to :Ms2ger from comment #23)
> My main question is this: will document.createElement("x-foo") or <x-foo> in
> markup be the expected way to use your own foo-thing? If so, we don't want
> to do that because the fallback and a11y story sucks.

Yes, the x- elements will use custom prototypes, but these custom prototypes are required to inheirt from the HTMLElement prototype.

What are the complications with fallback and a11y?
Comment 25 Daniel Buchner [:dbuc] 2012-09-26 13:45:40 PDT
(In reply to :Ms2ger from comment #23)
> My main question is this: will document.createElement("x-foo") or <x-foo> in
> markup be the expected way to use your own foo-thing? If so, we don't want
> to do that because the fallback and a11y story sucks.

Yes, this interface and output is far superior, and can inherit from existing prototypes. Additionally, role="" and other mechanisms can be employed to achieve similar results. Differentiating/inflating custom elements based on an attribute value is inappropriate and is a no-go in terms of expected API ergonomics - heck, most of the developers I've garnered feedback from asked if they can use custom tag names with no "x-"! (but we needed it to distinguish them, so we compromised)
Comment 26 :Ms2ger (⌚ UTC+1/+2) 2012-09-27 00:24:59 PDT
Alright, then this is unacceptable.
Comment 27 Jonas Sicking (:sicking) PTO Until July 5th 2012-09-27 00:52:32 PDT
This is not your decision alone. Please bring up the feedback to the spec or to the dev.platform newsgroup.

I haven't looked at any of the details here, so I don't have an opinion right now. But just closing the bug because you think the spec is bad is the wrong course of action.
Comment 28 Daniel Buchner [:dbuc] 2012-09-27 07:36:01 PDT
(In reply to :Ms2ger from comment #26)
> Alright, then this is unacceptable.

Your question was answered, and the ability to inherit from existing element prototypes and use the role attribute provides coverage for a11y. The attempt to close this enhancement with a dictatorial one-liner is troubling and unacceptable. If you would like to discuss this further, I am dbuc on IRC and can be found on the 3rd floor in Mountain View - otherwise, please allow William to continue with his work. (you're doing a great job William!)
Comment 29 William Chen [:wchen] 2012-10-15 14:45:14 PDT
(In reply to Blake Kaplan (:mrbkap) from comment #17)
> Comment on attachment 658066 [details] [diff] [review]
> WIP of document.register implementation
> 
> Review of attachment 658066 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have a couple of drive-by comments here. This looks really good though...
> I wonder if we might be able to land it if we throw NOT_IMPLEMENTED if
> someone passes us a shadow tree.
Done.
> 
> ::: content/base/src/nsDocument.cpp
> @@ +1579,5 @@
> >  
> > +  nsISupports* supports;
> > +  QueryInterface(NS_GET_IID(nsCycleCollectionISupports), reinterpret_cast<void**>(&supports));
> > +  NS_ASSERTION(supports, "Failed to QI to nsCycleCollectionISupports?!");
> > +  nsContentUtils::DropJSObjects(supports);
> 
> Is there any reason this can't use NS_DROP_JS_OBJECTS(this, nsDocument)?
> 
NS_DROP_JS_OBJECTS(this, nsDocument) casts to the wrong implementation of cycle collection participant for derived classes and that causes assertion failures.  
> @@ +2051,5 @@
> > +
> > +  nsISupports* thisSupports;
> > +  QueryInterface(NS_GET_IID(nsCycleCollectionISupports), reinterpret_cast<void**>(&thisSupports));
> > +  NS_ASSERTION(thisSupports, "Failed to QI to nsCycleCollectionISupports!");
> > +  rv = nsContentUtils::HoldJSObjects(thisSupports, participant);
> 
> And similarly, NS_HOLD_JS_OBJECTS here?
> 
Same as above.
> @@ +4639,5 @@
> > +  JSObject* callee = JSVAL_TO_OBJECT(JS_CALLEE(aCx, aVp));
> > +
> > +  nsIXPConnect* xpc = nsContentUtils::XPConnect();
> > +
> > +  jsval documentVal = js::GetFunctionNativeReserved(callee, 0);
> 
> Instead of using a reserved slot, you can simply use the function object's
> compartment's global (which will be a window) to get at the document.
> That'll simplify this a little, I think.
> 
Done.
> @@ +4646,5 @@
> > +  nsCOMPtr<nsIXPConnectWrappedNative> wrappedNative;
> > +  xpc->GetWrappedNativeOfJSObject(aCx, documentObj,
> > +                                  getter_AddRefs(wrappedNative));
> > +  nsCOMPtr<nsIDOMDocument> document =
> > +      do_QueryInterface(wrappedNative->Native());
> 
> This'll change if you make the above change, but for the record, this could
> be do_QueryWrappedNative.
> 
> @@ +4666,5 @@
> > +  JSObject* global = JS_GetGlobalForObject(aCx, callee);
> > +
> > +  jsval v;
> > +  rv = nsContentUtils::WrapNative(aCx, global, newElement,
> > +                                  (nsWrapperCache*) nullptr, &v);
> 
> Why do you need the cast?
> 
It's ambiguous without the cast.
> @@ +4715,5 @@
> > +  jsval customTemplate = JSVAL_VOID;
> > +
> > +  // Get optional arguments if provided.
> > +  if (aArgc > 0) {
> > +    NS_ENSURE_TRUE(!JSVAL_IS_PRIMITIVE(aOptions), NS_ERROR_UNEXPECTED);
> 
> You should probably use the HTML5 dictionary stuff to extract these values.
> 
Optional jsvals are not supported using the dictionary stuff.
> @@ +4741,5 @@
> > +                                     &protoObject);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +  } else {
> > +    NS_ENSURE_TRUE(!JSVAL_IS_PRIMITIVE(customProto) &&
> > +                   !JSVAL_IS_NULL(customProto),
> 
> Nit: JSVAL_IS_NULL is subsumed by the check for JSVAL_IS_PRIMITIVE. That
> being said, with a bit of extra work, you could also use the member
> functions on JS::Value, which are a little clearer and The Future (TM).
> 
Done.
> @@ +4756,5 @@
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +    // Check the proto chain for HTMLElement prototype.
> > +    JSObject* protoProto = JS_GetPrototype(protoObject);
> > +    bool isInChain = false;
> 
> Nit: isInChain isn't used.
> 
Done.
> @@ +4775,5 @@
> > +  // Do element upgrade.
> > +  nsRefPtr<nsContentList> list = GetElementsByTagName(lcName);
> > +  for (int32_t i = 0; i < list->Length(false); i++) {
> > +    nsINode* currentNode = list->Item(i, false);
> > +    nsCOMPtr<nsIDOMNode> oldNode = do_QueryInterface(currentNode);
> 
> C++ code (especially inside content/!) shouldn't use nsIDOMNode. Instead,
> use the functions available on nsINode.
> 
Done.
> @@ +4782,5 @@
> > +    }
> > +
> > +    // TODO(wchen): Hook up to templates when avilable.
> > +    nsCOMPtr<nsIDOMNode> newNode;
> > +    rv = oldNode->CloneNode(true, 1, getter_AddRefs(newNode));
> 
> So this would use nsINode::Clone.
> 
nsINode::Clone doesn't do what I want because I need a deep clone.
> @@ +4827,5 @@
> > +  // Put the document in the first reserved slot.
> > +  js::SetFunctionNativeReserved(constructorObject, 0, OBJECT_TO_JSVAL(GetWrapper()));
> > +
> > +  // Put element name in the second reserved slot.
> > +  JSString* elemName = JS_NewUCStringCopyZ(aCx, lcName.get());
> 
> Does the spec allow us to stick this info in the name of the function? If
> so, we can do away with the reserved slots, which would be nice.
> 
The function name isn't specified so I just stuck it in there. No more reserved slots.
> ::: dom/base/nsDOMClassInfo.cpp
> @@ +8256,5 @@
> > +  Element *element = static_cast<Element*>(wrapper->Native());
> > +  nsAutoString elementName;
> > +  nsContentUtils::ASCIIToLower(element->NodeName(), elementName);
> > +  if (StringBeginsWith(elementName, NS_LITERAL_STRING("x-"))) {
> > +    return PostCreate(wrapper, cx, obj);
> 
> Is this right? If I take an x-tag node and adopt it into another document,
> shouldn't it keep the same prototype?

That makes sense. Done.
Comment 30 William Chen [:wchen] 2012-10-15 14:46:04 PDT
Created attachment 671610 [details] [diff] [review]
Implementation of document.register without shadow DOM support. v2
Comment 31 William Chen [:wchen] 2012-10-15 14:46:30 PDT
Created attachment 671611 [details] [diff] [review]
v1 diff v2
Comment 32 Andrew McCreight [:mccr8] 2012-10-15 14:54:26 PDT
> NS_DROP_JS_OBJECTS(this, nsDocument) casts to the wrong implementation of cycle collection participant for derived classes and that causes assertion failures.

It might be worth it (in some other bug) to add some kind of NS_DROP_NSISUPPORTS_JS_OBJECTS(p) that does all the QIing.  PreserveWrapper needs the same thing.
Comment 33 Daniel Buchner [:dbuc] 2012-10-25 12:41:20 PDT
(In reply to William Chen [:wchen] from comment #31)
> Created attachment 671611 [details] [diff] [review]
> v1 diff v2

Any word on integration of Andrew's review recommendations, and anything else we need in order to land this?
Comment 34 William Chen [:wchen] 2012-10-26 11:22:52 PDT
(In reply to Daniel Buchner [:dbuc] from comment #33)
> (In reply to William Chen [:wchen] from comment #31)
> > Created attachment 671611 [details] [diff] [review]
> > v1 diff v2
> 
> Any word on integration of Andrew's review recommendations, and anything
> else we need in order to land this?

I'm not aware of any blockers for this so I don't think there is anything else. Andrew's comment is probably best addressed in a followup bug.
Comment 35 Blake Kaplan (:mrbkap) 2012-11-14 17:58:43 PST
Comment on attachment 671610 [details] [diff] [review]
Implementation of document.register without shadow DOM support. v2

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

I think I convinced Ben to sanity-check my review. I'm hoping he'll be around next week. If not, this will have to wait until after Thanksgiving to land.

::: content/base/src/nsDocument.cpp
@@ +4799,5 @@
> +
> +  JSObject* global = JS_GetGlobalForObject(aCx, callee);
> +  nsCOMPtr<nsIXPConnectWrappedNative> wrappedNative;
> +  xpc->GetWrappedNativeOfJSObject(aCx, global, getter_AddRefs(wrappedNative));
> +  nsCOMPtr<nsIDOMWindow> window = do_QueryWrappedNative(wrappedNative);

I missed this earlier, but this should use nsPIDOMWindow (the public DOM APIs are a mess of reference counting and out parameters) and can be simplified with: do_QueryWrapper(aCx, global);

@@ +4803,5 @@
> +  nsCOMPtr<nsIDOMWindow> window = do_QueryWrappedNative(wrappedNative);
> +  MOZ_ASSERT(window, "Should have a non-null window");
> +
> +  nsCOMPtr<nsIDOMDocument> document;
> +  window->GetDocument(getter_AddRefs(document));

So, I think this should be using nsIDocument (and window ->GetDoc()) in order to use the internal APIs instead of the DOM ones.

@@ +4814,5 @@
> +  }
> +
> +  nsCOMPtr<nsIDOMElement> newElement;
> +  nsresult rv = document->CreateElement(elemName,
> +                                        getter_AddRefs(newElement));

And then this gets a little more complicated as it needs to use CreateElem.
Comment 36 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-11-29 15:50:47 PST
Comment on attachment 671610 [details] [diff] [review]
Implementation of document.register without shadow DOM support. v2

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

This looks good! Some nits below but nothing major.

::: content/base/src/nsDocument.cpp
@@ +1912,5 @@
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>  
>  
> +struct CustomPrototypeTraceArgs {
> +  TraceCallback& callback;

Nit: No need for the & here.

@@ +1916,5 @@
> +  TraceCallback& callback;
> +  void* closure;
> +};
> +
> +

Nit: two newlines here and in a few places below

@@ +4806,5 @@
> +  nsCOMPtr<nsIDOMDocument> document;
> +  window->GetDocument(getter_AddRefs(document));
> +
> +  // Function name is the type of the custom element.
> +  JSString* jsFunName = JS_GetFunctionId(JS_ValueToFunction(aCx, *aVp));

It looks like you're duplicating JS_CALLEE here.

@@ +4816,5 @@
> +  nsCOMPtr<nsIDOMElement> newElement;
> +  nsresult rv = document->CreateElement(elemName,
> +                                        getter_AddRefs(newElement));
> +
> +  jsval v;

Nit: Elsewhere you're using JS::Value.

@@ +4840,5 @@
> +  nsresult rv = xpc->GetWrappedNativePrototype(aCx, aScope, classInfo,
> +                                               getter_AddRefs(holder));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  JSObject* interface = nullptr;

Nit: No need to initialize this.

@@ +4865,5 @@
> +
> +  JS::Value customProto = JSVAL_VOID;
> +  JS::Value customTemplate = JSVAL_VOID;
> +
> +  JSBool success;

Nit: Just bool here.

@@ +4870,5 @@
> +  // Get optional arguments if provided.
> +  if (aArgc > 0) {
> +    NS_ENSURE_TRUE(!aOptions.isPrimitive(), NS_ERROR_UNEXPECTED);
> +    success = JS_GetProperty(aCx, &aOptions.toObject(), "prototype",
> +                             &customProto);

This should use the dictionary stuff like Blake said. What kind of trouble were you having when you tried to use it?

@@ +4877,5 @@
> +                             &customTemplate);
> +    NS_ENSURE_TRUE(success, NS_ERROR_UNEXPECTED);
> +  }
> +
> +  // TODO(wchen): Templates are not currently supported.

Nit: Include bug number for templates here.

@@ +4886,5 @@
> +  nsIScriptGlobalObject* sgo = GetScopeObject();
> +  NS_ENSURE_TRUE(sgo, NS_ERROR_UNEXPECTED);
> +  JSObject* global = sgo->GetGlobalJSObject();
> +
> +  JSContext* cx = nsContentUtils::GetContextFromDocument(this);

Hm, you probably just want to keep using aCx, right?

@@ +4938,5 @@
> +    nsCOMPtr<nsINode> newNode;
> +    rv = nsNodeUtils::Clone(oldNode, true, getter_AddRefs(newNode));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsINode* parentNode = oldNode->GetNodeParent();

Nit: This must be non-null if GetElementsByTagName returned it. Assert.

@@ +4939,5 @@
> +    rv = nsNodeUtils::Clone(oldNode, true, getter_AddRefs(newNode));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsINode* parentNode = oldNode->GetNodeParent();
> +    nsCOMPtr<nsIDOMElement> newElement = do_QueryInterface(newNode);

Nit: This should just be asserted, not checked below.

@@ +4949,5 @@
> +      nsCOMPtr<nsIDOMEvent> event;
> +      rv = CreateEvent(NS_LITERAL_STRING("elementreplace"), getter_AddRefs(event));
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      nsCOMPtr<nsIDOMElementReplaceEvent> ptEvent = do_QueryInterface(event);

This should just be asserted.

@@ +4952,5 @@
> +
> +      nsCOMPtr<nsIDOMElementReplaceEvent> ptEvent = do_QueryInterface(event);
> +      if (ptEvent) {
> +        rv = ptEvent->InitElementReplaceEvent(
> +            NS_LITERAL_STRING("elementreplace"), false, false, newElement);

Nit: Strange indent

@@ +4970,5 @@
> +
> +  // Create constructor to return. Store the name of the custom element as the
> +  // name of the function.
> +  JSFunction* constructor = JS_NewFunction(aCx, CustomElementConstructor, 0,
> +                                           JSFUN_CONSTRUCTOR, 0,

Nit: nullptr for the parent arg

::: dom/base/nsDOMClassInfo.cpp
@@ +8099,5 @@
> +  nsContentUtils::ASCIIToLower(element->NodeName(), elementName);
> +  if (StringBeginsWith(elementName, NS_LITERAL_STRING("x-"))) {
> +    nsDocument* document = static_cast<nsDocument*>(element->OwnerDoc());
> +    JSObject* prototype = nullptr;
> +    document->mCustomPrototypes.Get(elementName, &prototype);

I think this should be encapsulated better (and let's make sure that mCustomPrototypes is not public).

@@ +8100,5 @@
> +  if (StringBeginsWith(elementName, NS_LITERAL_STRING("x-"))) {
> +    nsDocument* document = static_cast<nsDocument*>(element->OwnerDoc());
> +    JSObject* prototype = nullptr;
> +    document->mCustomPrototypes.Get(elementName, &prototype);
> +    if (prototype) {

This can just be:

  JSObject* prototype;
  if (document->mCustomPrototypes.Get(elementName, &prototype)) {
    ...
  }

::: dom/interfaces/core/nsIDOMDocument.idl
@@ +27,4 @@
>   * http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html
>   */
>  
> +[scriptable, uuid(8D73513A-FD62-4BD0-96E7-BB8DB77BBDB5)]

Nit: lowercase
Comment 37 William Chen [:wchen] 2012-12-06 11:56:11 PST
(In reply to Blake Kaplan (:mrbkap) from comment #35)
> Comment on attachment 671610 [details] [diff] [review]
> Implementation of document.register without shadow DOM support. v2
> 
> Review of attachment 671610 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think I convinced Ben to sanity-check my review. I'm hoping he'll be
> around next week. If not, this will have to wait until after Thanksgiving to
> land.
> 

All comments addressed.

(In reply to ben turner [:bent] from comment #36)
> Comment on attachment 671610 [details] [diff] [review]
> Implementation of document.register without shadow DOM support. v2
> 
> Review of attachment 671610 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good! Some nits below but nothing major.
> 

Add comments addressed except for:

> @@ +4806,5 @@
> > +  nsCOMPtr<nsIDOMDocument> document;
> > +  window->GetDocument(getter_AddRefs(document));
> > +
> > +  // Function name is the type of the custom element.
> > +  JSString* jsFunName = JS_GetFunctionId(JS_ValueToFunction(aCx, *aVp));
> 
> It looks like you're duplicating JS_CALLEE here.
> 
I don't see what you mean here.

> @@ +4870,5 @@
> > +  // Get optional arguments if provided.
> > +  if (aArgc > 0) {
> > +    NS_ENSURE_TRUE(!aOptions.isPrimitive(), NS_ERROR_UNEXPECTED);
> > +    success = JS_GetProperty(aCx, &aOptions.toObject(), "prototype",
> > +                             &customProto);
> 
> This should use the dictionary stuff like Blake said. What kind of trouble
> were you having when you tried to use it?
> 
The problem is that the dictionary stuff does not parse a nullable jsval member.

The spec changed since the last time I uploaded a patch, so there is now another piece that needs to be reviewed. I'll upload a patch with the diff implementing the lifecycle callback.
Comment 38 William Chen [:wchen] 2012-12-06 11:57:07 PST
Created attachment 689326 [details] [diff] [review]
Implementation of document.register without shadow DOM support. v3
Comment 39 William Chen [:wchen] 2012-12-06 11:57:42 PST
Created attachment 689327 [details] [diff] [review]
v2 diff v3
Comment 40 Blake Kaplan (:mrbkap) 2012-12-06 15:05:02 PST
(In reply to William Chen [:wchen] from comment #37)
> > > +  JSString* jsFunName = JS_GetFunctionId(JS_ValueToFunction(aCx, *aVp));
> > 
> > It looks like you're duplicating JS_CALLEE here.

*aVp is JS_CALLEE which you used earlier.
Comment 41 William Chen [:wchen] 2012-12-06 16:54:23 PST
Created attachment 689472 [details] [diff] [review]
Implementation of document.register without shadow DOM support. v4

Using dictionary and fixed JS_CALLEE use.
Comment 42 :Ms2ger (⌚ UTC+1/+2) 2012-12-07 02:47:14 PST
Use JS::CallArgs if you're implementing JSNatives.
Comment 43 William Chen [:wchen] 2012-12-12 10:39:38 PST
Created attachment 691432 [details] [diff] [review]
WebIDL version of implementation.

Here is the WebIDL version of the patch. Although this one has some WebIDL dependencies.
Comment 44 Blake Kaplan (:mrbkap) 2012-12-17 16:59:53 PST
Comment on attachment 689472 [details] [diff] [review]
Implementation of document.register without shadow DOM support. v4

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

Let's get this in sooner rather than later and switch to the webidl impl once the remaining dependencies there have been fixed.

::: content/base/src/nsDocument.cpp
@@ +4623,5 @@
>  
> +static JSBool
> +CustomElementConstructor(JSContext *aCx, unsigned aArgc, JS::Value* aVp)
> +{
> +  JS::Value calleeVal = JS_CALLEE(aCx, aVp);

As a followup or before you land, it's probably worth using JS::CallArgs instead of the JS_* macros for aVp as Ms2ger pointed out.

::: dom/interfaces/core/nsIDOMDocument.idl
@@ +49,5 @@
>  
> +  [optional_argc,
> +   implicit_jscontext] jsval    register(in DOMString name,
> +                                         [optional] in jsval options)
> +                                  raises(DOMException);

Need to bump the IID here.
Comment 46 Dimitri Glazkov 2012-12-21 14:26:06 PST
You guys rock.
Comment 47 Boris Zbarsky [:bz] 2012-12-21 15:03:37 PST
Argh.  Need to port this to WebIDL so we're not doing so much manual JSAPI crap somehow.  :(  I'm not sure how yet; see my questions on public-script-coords about IDL for things like static functions...  Or maybe we can't even do it at all.  :(

One question: As far as I can tell if CustomElementConstructor is called while aCx is not in the document's global's compartment it will return things in the wrong compartment.  What prevents such a call from happening, exactly?  What happens if one registers things through an Xray?
Comment 48 Boris Zbarsky [:bz] 2012-12-21 15:08:41 PST
In fact... if register is called via an xray, won't we have compartment mismatches just because "global" and aCx are in different compartments?
Comment 49 Boris Zbarsky [:bz] 2012-12-21 15:24:57 PST
On that note, we should store the custom protos in the compartment of the document's global, I think.  Otherwise you get mismatches when creating elements.

And we need to figure out some way to not have use of this completely break the TI optimizations IonMonkey does for elements, which right now it would.  :(
Comment 50 Boris Zbarsky [:bz] 2012-12-21 15:28:06 PST
And have we done any perf measurement on whether the dynamic proto set deoptimizes things on elements?

I know for a fact that creating random non-singleton-class objects whose proto is HTMLElement.prototype _will_ deoptimize all HTMLElement methods.  Please get a followup bug filed on that so we can fix TI do deal with it somehow?
Comment 51 Daniel Buchner [:dbuc] 2012-12-21 15:35:15 PST
Someone might want to speak to Dimitri - while I'm not sure if it dealt with the same code concerns (obviously their code base is quite different) - he was saying the Google guys were actually realizing massive perf *gains* on custom elements.
Comment 52 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-12-21 15:51:14 PST
Backed out due to test failures across the board:
https://hg.mozilla.org/integration/mozilla-inbound/rev/972c3e4a494f

214 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/webcomponents/test_document_register.html | uncaught exception - TypeMismatchError: The type of an object is incompatible with the expected type of the parameter associated to the object at http://mochi.test:8888/tests/dom/tests/mochitest/webcomponents/test_document_register.html:55
217 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/webcomponents/test_document_register_lifecycle.html | uncaught exception - TypeMismatchError: The type of an object is incompatible with the expected type of the parameter associated to the object at http://mochi.test:8888/tests/dom/tests/mochitest/webcomponents/test_document_register_lifecycle.html:32
Comment 53 Dimitri Glazkov 2012-12-21 15:53:48 PST
(In reply to Daniel Buchner [:dbuc] from comment #51)
> Someone might want to speak to Dimitri - while I'm not sure if it dealt with
> the same code concerns (obviously their code base is quite different) - he
> was saying the Google guys were actually realizing massive perf *gains* on
> custom elements.

I can't remember massive perf gains discussion, but Morrita (morrita@google.com) and Dominic (dominicc@google.com) have studied the problem for WebKit/V8 extensively. They'd be great peeps to talk about.
Comment 54 Jeff Walden [:Waldo] (remove +bmo to email) 2012-12-21 16:00:26 PST
(In reply to Daniel Buchner [:dbuc] from comment #51)
> he was saying the Google guys were actually realizing massive perf *gains*
> on custom elements.

Yes, but I think bz was saying that this would deoptimize calls to existing HTMLElement.prototype.* and Element.prototype.* functions when made with non-custom elements.  Making custom stuff faster at the cost of slowing everything else down wouldn't be cool.  (It's worth noting that our DOM method JITting implementation isn't like anyone else's, I believe, so there's a limit to how much other-implementation experience can be helpful here.)

Which all assumes I'm intuiting things properly here about what was meant.  No opinions one way or another on any of the rest of this, as I haven't looked at semantics of anything at all to know if it's sane or crazy either.
Comment 55 Boris Zbarsky [:bz] 2012-12-21 16:16:12 PST
Jeff is right.  V8 and JSC both violate the WebIDL spec and put properties directly on nodes, not on prototypes to speed them up, so their implementation experience is somewhat irrelevant here because they're solving a totally different problem.  We're implementing the spec properly, but relying on type inference in the JIT to make DOM properties fast.  But the type inference depends on all objects whose proto is a DOM prototype either being actual DOM objects or being flagged as singleton-type objects (which random DOM prototypes are, but random other objects are not).  So as soon as you create a random object whose proto is HTMLElement.prototype, all HMLElement.prototype functionality on all elements in that document gets slower.

So we need to fix things somehow so that won't happen.  Long-term there's a plan involving tying the type inference to JSClass instead of prototype, but that depends on some changes to the array implementation which Jeff can tell you all about at length if you really care.  ;)  But that's a pretty long-term plan; we may want a shorter-term solution.

Past that, I'm going to attach a small patch that fixes the orange from comment 52.  In the process of writing this 5-line patch I filed 3 spec bugs before my fingers got tired.  :(  In general, we really need to go through the spec and file bugs on all the parts of it that are too vague to implement interoperably, which so far is pretty much every sentence I've read.  :(
Comment 56 Boris Zbarsky [:bz] 2012-12-21 16:20:23 PST
Created attachment 695048 [details] [diff] [review]
Patch that hacks around the orange

The nsDocument part of this is probably ready for review.

The other part is not the same as what nsElementSH::PostCreate does, and neither one is correct, probably.  But since the spec doesn't actually define behavior there, I have no idea what the code _should_ be...

In particular, consider exciting things like document.createElementNS("http://www.w3.org/1999/xhtml", "x-foo:x-bar") and whatnot.
Comment 57 Boris Zbarsky [:bz] 2012-12-21 17:02:09 PST
A few last issues:

1) The upgrade code needs to hold a strong ref to oldNode.  Otherwise it might be dead
   after the replaceChild call, but then we use it after that.

2) Defining "prototype" and "constructor" via JS_SetProperty is wrong.  I know that's what
   the spec says to do; it's still wrong.
   See https://www.w3.org/Bugs/Public/show_bug.cgi?id=20493

3) If the plan is for LifecycleCallbacks to become a callback function, we can just do
   that via WebIDL now and nuke all the this-translator bits... Is that the plan?

4) Why do we not want to allow slimwrappers for custom elements?  Is this something we
   need to watch out for in the WebIDL bindings?  I don't see anything in this patch that
   relies on XPCWrappedNatives, so I'm not sure why we need that bit.  Or is that just
   needs so PostCreate will be triggered at all?
Comment 58 William Chen [:wchen] 2013-02-25 22:14:30 PST
Created attachment 718256 [details] [diff] [review]
Implementation of document.register without shadow DOM support. v5

I've rebased the patch and put the feature behind a pref. I've updated it so that it's aligned closer to the current spec (but not completely). We can follow up with another bug to update the implementation when the spec is more stable, but for now I would like to get this patch in so that I don't have to deal with bitrot.

(In reply to Boris Zbarsky (:bz) from comment #57)
> 
> 3) If the plan is for LifecycleCallbacks to become a callback function, we
> can just do
>    that via WebIDL now and nuke all the this-translator bits... Is that the
> plan?
> 
That has gone away now with WebIDL.
> 4) Why do we not want to allow slimwrappers for custom elements?  Is this
> something we
>    need to watch out for in the WebIDL bindings?  I don't see anything in
> this patch that
>    relies on XPCWrappedNatives, so I'm not sure why we need that bit.  Or is
> that just
>    needs so PostCreate will be triggered at all?
Yes, that was the reason. But this has also gone away with WebIDL.

I've addressed the other code comments.
Comment 59 William Chen [:wchen] 2013-02-25 22:15:06 PST
Created attachment 718259 [details] [diff] [review]
interdiff
Comment 60 Boris Zbarsky [:bz] 2013-02-26 07:37:47 PST
>+  rv = nsContentUtils::WrapNative(aCx, global, newElement,
>+                                  (nsWrapperCache*) nullptr, &v);

Why not just pass newElement for the nsWrapperCache*?

In the XPCOM Register(), I'd prefer s/aArgc/aOptionalArgc/.

In the WebIDL Register, are we guaranteed that aOptions.mPrototype is in the same compartment as aCx?  I don't think we are: when called via WebIDL Xrays, aOptions.mPrototype is in the caller compartment, but we entered the compartment of "global" on aCx.  I think we need a JS_WrapObject(aCx, &protoObject) right after we do |protoObject = aOptions.mPrototype|.  I think we should be OK after that, assuming JS_GetPrototype does JS_WrapObject as needed or that the protos of cross-compartment wrappers are otherwise sane (in the sense of being cross-compartment wrappers for the proto of the wrappee).  Bobby, are they?

The loop there looks like it'll fail if I call register() on document A but pass the HTMLElement.prototype from some other window.  It's not clear to me what should happen in that case per spec; please raise a spec issue.  The fact that registration is per-document but interface inheritance is global is a serious impedance mismatch for the way the spec is currently written....

We reparent a document's wrapper on document.open, right?  What should happen to mCustomPrototypes in that situation?  I would say we should clear them, fwiw.  Perhaps generally as part of Reset?

>+      aOptions.mLifecycle.mCreated->Call((nsISupports*) newElement, rv);

Why do you need the cast there?  I don't think you do....

I don't see where the spec actually specifies that exceptions thrown by created callbacks terminate the upgrade algorithm.  I would argue that they shouldn't, in fact.

>+  NS_ENSURE_TRUE(JS_WrapObject(aCx, &constructorObject), nullptr);

This is a no-op: the function was just created on aCx, so it's already in the right compartment.  Just take this line out, please.

>+    if (IsHTML() && Substring(NodeName(), 0, 2).LowerCaseEqualsLiteral("x-")) {

This is HTMLUnknownElement, so it's definitely IsHTML().  ;)

>+      JSAutoCompartment ac(aCx, global);

Why is this not equivalent to:

  JSAutoCompartment ac(aCx, obj);

without having to get the global and whatnot?  Furthermore, once we do that, I would expect the thing GetCustomPrototype returns (which is in the compartment of the document's wrapper; I believe we should document that on mCustomPrototypes), to already be in the resulting compartment.

This is looking way better, thank you!
Comment 61 Bobby Holley (:bholley) (busy with Stylo) 2013-02-26 08:28:53 PST
(In reply to Boris Zbarsky (:bz) from comment #60)
> assuming JS_GetPrototype does JS_WrapObject as needed

It does not.

> that the protos of cross-compartment wrappers are otherwise sane (in the
> sense of being cross-compartment wrappers for the proto of the wrappee).
> Bobby, are they?

Generally, yes. We sometimes do special stuff, but the proto will always be same-compartment with the base object.
Comment 62 Boris Zbarsky [:bz] 2013-02-26 08:33:32 PST
> but the proto will always be same-compartment with the base object.

That's good enough for the purposes of this code.
Comment 63 Daniel Buchner [:dbuc] 2013-02-26 09:03:06 PST
(In reply to Boris Zbarsky (:bz) from comment #60)
> >+    if (IsHTML() && Substring(NodeName(), 0, 2).LowerCaseEqualsLiteral("x-")) {

If this is looking for x- as a prefix, which it appears to, I should note the spec was changed to be keyed on the presence of a dash anywhere in the tag name. Does the patch take this into account?
Comment 64 Boris Zbarsky [:bz] 2013-02-26 09:17:56 PST
> Does the patch take this into account?

No.  See comment 58: we're not trying to chase the rapidly mutating and totally unstable spec so far.  We'll worry about it once it stops doing that.
Comment 65 William Chen [:wchen] 2013-02-26 11:31:04 PST
Created attachment 718546 [details] [diff] [review]
Implementation of document.register without shadow DOM support. v6

(In reply to Boris Zbarsky (:bz) from comment #60)
> The loop there looks like it'll fail if I call register() on document A but
> pass the HTMLElement.prototype from some other window.  It's not clear to me
> what should happen in that case per spec; please raise a spec issue.  The
> fact that registration is per-document but interface inheritance is global
> is a serious impedance mismatch for the way the spec is currently written....
> 
Ok, I'll create a spec bug for this.
> 
> >+      aOptions.mLifecycle.mCreated->Call((nsISupports*) newElement, rv);
> 
> Why do you need the cast there?  I don't think you do....
> 
Ambiguity between GetWrapperCache(void* p) and GetWrapperCache(const ParentObject& aParentObject) a few levels deep in template instantiation (Call -> WrapCallThisObject -> WrapNativeParent -> GetWrapperCache).

I've addressed the comments.
Comment 66 William Chen [:wchen] 2013-02-26 11:31:42 PST
Created attachment 718547 [details] [diff] [review]
v5 diff v6
Comment 67 Boris Zbarsky [:bz] 2013-02-26 11:32:00 PST
> Ambiguity between GetWrapperCache(void* p) and GetWrapperCache(const ParentObject&
> aParentObject)

Ah.  Does passing newElement.get() help?
Comment 68 Boris Zbarsky [:bz] 2013-02-26 11:33:54 PST
>+    NS_ENSURE_TRUE(JS_WrapObject(aCx, &protoObject), nullptr);

You probably want to throw something on rv in that case; I think you'll crash in binding code with a null deref otherwise.

Thanks for the quick response!
Comment 69 William Chen [:wchen] 2013-02-26 11:38:32 PST
(In reply to Boris Zbarsky (:bz) from comment #67)
> > Ambiguity between GetWrapperCache(void* p) and GetWrapperCache(const ParentObject&
> > aParentObject)
> 
> Ah.  Does passing newElement.get() help?

Yes, that does the trick.
(In reply to Boris Zbarsky (:bz) from comment #68)
> >+    NS_ENSURE_TRUE(JS_WrapObject(aCx, &protoObject), nullptr);
> 
> You probably want to throw something on rv in that case; I think you'll
> crash in binding code with a null deref otherwise.
> 
> Thanks for the quick response!

OK, done locally.
Comment 70 Boris Zbarsky [:bz] 2013-02-26 11:46:07 PST
> Yes, that does the trick.

OK.  This is ridiculous; I ran into it in bug 810644 too.  We should just add overloads of GetWrapperCache for nsCOMPtr<T> and nsRefPtr<T>.  Want to do it, or should I?
Comment 71 William Chen [:wchen] 2013-02-26 11:53:48 PST
(In reply to Boris Zbarsky (:bz) from comment #70)
> > Yes, that does the trick.
> 
> OK.  This is ridiculous; I ran into it in bug 810644 too.  We should just
> add overloads of GetWrapperCache for nsCOMPtr<T> and nsRefPtr<T>.  Want to
> do it, or should I?
I can do this.
Comment 72 Boris Zbarsky [:bz] 2013-02-26 12:09:43 PST
Followup is fine for that, just to be clear.
Comment 73 Blake Kaplan (:mrbkap) 2013-03-04 13:55:27 PST
Comment on attachment 718546 [details] [diff] [review]
Implementation of document.register without shadow DOM support. v6

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

A couple of small comments, but nothing that should stop this from landing.

::: content/base/public/nsIDocument.h
@@ +1866,5 @@
>    {
>      return GetRootElement();
>    }
> +  virtual JSObject*
> +    Register(JSContext* aCx, const nsAString& aName,

Nit: The "R" in register should be directly under the "v" above it.

::: content/base/src/nsDocument.cpp
@@ +4974,5 @@
> +                     mozilla::ErrorResult& rv)
> +{
> +  nsAutoString lcName;
> +  nsContentUtils::ASCIIToLower(aName, lcName);
> +  if (Substring(NodeName(), 0, 2).LowerCaseEqualsLiteral("x-")) {

NodeName() isn't right here. Also, this should be able to use

if (!StringBeginsWith(lcName, "x-")) {
  ...
}

I think the sense of the if statement is wrong here, as well.

::: content/html/content/src/HTMLUnknownElement.cpp
@@ +21,5 @@
>  {
> +  JSObject* obj =
> +    HTMLUnknownElementBinding::Wrap(aCx, aScope, this, aTriedToWrap);
> +  if (obj) {
> +    if (Substring(NodeName(), 0, 2).LowerCaseEqualsLiteral("x-")) {

This can also use StringBeginsWith.
Comment 75 Ryan VanderMeulen [:RyanVM] 2013-03-05 07:42:34 PST
https://hg.mozilla.org/mozilla-central/rev/871fea464883
Comment 76 Daniel Buchner [:dbuc] 2013-03-05 07:53:02 PST
(In reply to William Chen [:wchen] from comment #74)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/871fea464883

Did you just land this William?!? --> "Did we just become best friends?"
Comment 77 William Chen [:wchen] 2013-03-05 12:21:05 PST
*** Bug 824151 has been marked as a duplicate of this bug. ***
Comment 78 Daniel Buchner [:dbuc] 2013-03-10 22:21:48 PDT
(In reply to Blake Kaplan (:mrbkap) from comment #73)

Any reason this patch hasn't hit nightly yet? If there's a blocking issue, what would a ballpark ETA be for it?
Comment 79 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-03-10 22:44:49 PDT
(In reply to Daniel Buchner [:dbuc] from comment #78)
> (In reply to Blake Kaplan (:mrbkap) from comment #73)
> 
> Any reason this patch hasn't hit nightly yet? If there's a blocking issue,
> what would a ballpark ETA be for it?

It's been in Nightly for several days ...
Comment 80 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-03-10 22:49:25 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #79)
> (In reply to Daniel Buchner [:dbuc] from comment #78)
> > (In reply to Blake Kaplan (:mrbkap) from comment #73)
> > 
> > Any reason this patch hasn't hit nightly yet? If there's a blocking issue,
> > what would a ballpark ETA be for it?
> 
> It's been in Nightly for several days ...

Perhaps you're not setting the pref?  Set dom.webcomponents.enabled to true and restart.
Comment 81 Daniel Buchner [:dbuc] 2013-03-11 10:39:12 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #80)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #79)
> > (In reply to Daniel Buchner [:dbuc] from comment #78)
> > > (In reply to Blake Kaplan (:mrbkap) from comment #73)
> > > 
> > > Any reason this patch hasn't hit nightly yet? If there's a blocking issue,
> > > what would a ballpark ETA be for it?
> > 
> > It's been in Nightly for several days ...
> 
> Perhaps you're not setting the pref?  Set dom.webcomponents.enabled to true
> and restart.

I knew the pref, but thought it was in there by default vs needed to be created, silly me - thank you Kyle!
Comment 82 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2013-03-11 12:28:54 PDT
Nit: This landed with an old datestamp in the patch-headers, so the commits from comment 74 & comment 76 show up on hgweb with this date (from > 5 months ago) associated with it:
> Thu Nov 01 11:18:08 2012 -0700 (at Thu Nov 01 11:18:08 2012 -0700)

Ideally, it's nice for a patch's datestamp to be near the time when it actually landed.

You can achieve this either by simply never adding a datestamp (until it's actually qfinished, at which point the datestamp is automatically added), or just run "hg qref -D" on the patch before qfinishing.

Not a big deal, just a heads-up for future reference.
Comment 83 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2013-03-11 12:29:45 PDT
(In reply to Daniel Holbert [:dholbert] from comment #82)
> Nit: This landed with an old datestamp in the patch-headers, so the commits
> from comment 74 & comment 76 show up on hgweb with this date

(er "from comment 74 & comment 75")

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