Closed Bug 783129 Opened 12 years ago Closed 11 years ago

Implement the document.register interface method

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla22

People

(Reporter: dbuchner, Assigned: wchen)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-needed)

Attachments

(3 files, 9 obsolete files)

interdiff
19.54 KB, patch
Details | Diff | Splinter Review
Implementation of document.register without shadow DOM support. v6
41.92 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
v5 diff v6
6.83 KB, patch
Details | Diff | Splinter Review 
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.
Well, the template part needs support for Shadow DOM.
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.
(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.
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.
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.
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...
(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?
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.
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)
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.
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.
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?
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.
(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?
Can we move discussion to spec bug? :) I don't want to lose valuable questions/insight from you folks.
Here is a WIP that implements what we can without support for templates and shadow DOM.
Assignee: nobody → wchen
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?
Huh, why are we doing anything with x-tags? We decided we wouldn't do that.
This is document.register, which is a related API but bound to Shadow DOM. When I said x-tags I meant a "registered element."
But then I saw

if (StringBeginsWith(elementName, NS_LITERAL_STRING("x-"))) {
(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-".
(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?
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.
(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?
(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)
Alright, then this is unacceptable.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(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!)
(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.

        Attachment #658066 -
        Attachment is obsolete: true

        Attachment #671610 -
        Flags: review?(mrbkap)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1 diff v2 (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  
> 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.

    
    

    
  
(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?

    
    

    
  
(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 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.

        Attachment #671610 -
        Flags: review?(mrbkap)

        Attachment #671610 -
        Flags: review?(bent.mozilla)

        Attachment #671610 -
        Flags: review+

    
    

    
  
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

        Attachment #671610 -
        Flags: review?(bent.mozilla) → review+

    
    

    
  
(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.

    
    

    
  


  

        Attachment #671610 -
        Attachment is obsolete: true

        Attachment #689326 -
        Flags: review?(mrbkap)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v2 diff v3 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #671611 -
        Attachment is obsolete: true

    
    

    
  
(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.
URL: http://dvcs.w3.org/hg/webcomponents/r...

    
    

    
  


  
Using dictionary and fixed JS_CALLEE use.

        Attachment #689326 -
        Attachment is obsolete: true

        Attachment #689326 -
        Flags: review?(mrbkap)

        Attachment #689472 -
        Flags: review?(mrbkap)

    
    

    
  
Use JS::CallArgs if you're implementing JSNatives.

    
    

    
  


  
Here is the WebIDL version of the patch. Although this one has some WebIDL dependencies.

    
  
Depends on: 820544

    
  
Depends on: 820957

    
    

    
  
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.

        Attachment #689472 -
        Flags: review?(mrbkap) → review+

    
    

    
  
You guys rock.

    
    

    
  
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?

    
    

    
  
In fact... if register is called via an xray, won't we have compartment mismatches just because "global" and aCx are in different compartments?

    
    

    
  
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.  :(

    
    

    
  
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?

    
    

    
  
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.

    
    

    
  
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

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
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.  :(

    
    

    
  


  
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.
Blocks: 824151

    
    

    
  
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?

    
  
Depends on: 828532

    
    

    
  


  
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.

        Attachment #689327 -
        Attachment is obsolete: true

        Attachment #689472 -
        Attachment is obsolete: true

        Attachment #691432 -
        Attachment is obsolete: true

        Attachment #695048 -
        Attachment is obsolete: true

        Attachment #718256 -
        Flags: review?(mrbkap)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        interdiff
        — 
          — Splinter Review
      

    


  

    
    

    
  
>+  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!
Flags: needinfo?(bobbyholley+bmo)

    
    

    
  
(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.
Flags: needinfo?(bobbyholley+bmo)

    
    

    
  
> but the proto will always be same-compartment with the base object.

That's good enough for the purposes of this code.

    
    

    
  
(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?

    
    

    
  
> 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.

    
    

    
  


  
(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.

        Attachment #718256 -
        Attachment is obsolete: true

        Attachment #718256 -
        Flags: review?(mrbkap)

        Attachment #718546 -
        Flags: review?(mrbkap)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v5 diff v6
        — 
          — Splinter Review
      

    


  

    
    

    
  
> Ambiguity between GetWrapperCache(void* p) and GetWrapperCache(const ParentObject&
> aParentObject)

Ah.  Does passing newElement.get() help?

    
    

    
  
>+    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!

    
    

    
  
(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.

    
    

    
  
> 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?

    
    

    
  
(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.

    
    

    
  
Followup is fine for that, just to be clear.

    
    

    
  
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.

        Attachment #718546 -
        Flags: review?(mrbkap) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/871fea464883
Status: REOPENED → RESOLVED
Closed: 12 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22

    
    

    
  
(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?"

    
    

    
  
(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?

    
    

    
  
(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 ...

    
    

    
  
(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.

    
    

    
  
(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!
Depends on: 849866
Version: unspecified → Trunk

    
    

    
  
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.

    
    

    
  
(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")
Component: DOM → DOM: Core & HTML

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: