825628 - Implement NamedConstructor

Status: RESOLVED FIXED

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

Description

[(no longer active)]

We need this in order to properly implement HTMLImageElement (see bug 825527).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Need to decide whether to set this up when we set up the proto and interface object or off a separate resolve hook kinda thing...

The former seems somewhat simpler; we'd just pass in arrays of ctor data instead of a single set of ctor data.  That said, we'll still need to resolve the name in the window resolve hook, of course.

Peter, thoughts?

Comment 2

[Peter Van der Beken [:peterv]]

I'd do it eagerly, not off a resolve hook.

> That said, we'll still need to
> resolve the name in the window resolve hook, of course.

Yeah, I think calling RegisterDefineDOMInterface for each of the names should work. We'll need to add eTypeExternalConstructorAlias to http://hg.mozilla.org/mozilla-central/annotate/a812ef63de87/dom/base/nsDOMClassInfo.cpp#l6298 and figure out where to store the different constructors and how to get them (right now the protoAndIfaceArray stores at most one constructor without any name).

Comment 3

[Boris Zbarsky [:bzbarsky]]

> We'll need to add eTypeExternalConstructorAlias ... and figure out where to store the
> different constructors and how to get them

Why do we need to store the different constructors?

As I see it, the logic should be somewhat like this:

1)  Eagerly install the named constructors when setting up the interface objects for an
    interface.
2)  When asked to resolve the name, check for a cached proto object on the global for that
    interface.  If it's present, just return null: that means someone explicitly deleted
    the named constructor, so we shouldn't re-resolve it.  Otherwise, call the relevant
    binding's CreateInterfaceObjects, which will define the property for us.

In fact, I'm not quite sure why we don't do it that way for interface objects right now.  Is that just due to the defineOnXray bit in nsWindowSH::GlobalResolve?

Comment 4

[Peter Van der Beken [:peterv]]

Right on the Xray bit. I'd rather not move the Xray stuff into the CreateInterfaceObjects again, but something has to define these properties on the Xray (holder object).

Comment 5

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #3)
> Why do we need to store the different constructors?

Regardless of what defines the properties on the Xray we'll need to store the constructors somewhere. We can't rely on whether we have a cached proto object, there can be multiple Xrays for a window so we'd end up only defining it on the first Xray that we resolve on (at most).

One thing we could do is put the constructors in a linked list hanging off of the interface object that's stored in the array.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yeah, I guess that would work.  We'd have to be careful in the code that traces that stuff...

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: Add codegen support for NamedConstructor v1

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: Use NamedConstructor for Image() and Option() v1

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 715236 [details] [diff] [review]
Add codegen support for NamedConstructor v1

We need to either assert or better yet ensure that any cases with named constructors get a interface object that's not actually just a function, since we're depending on its reserved slot bits.

>+++ b/dom/bindings/BindingUtils.cpp
>+jsid s_constructor_id = JSID_VOID;
>+jsid s_length_id = JSID_VOID;
>+jsid s_prototype_id = JSID_VOID;

We can't do that, I don't think.

jsids are per-runtime, so if we have any code that might run in both workers and non-workers (which seems like it would be the case for named constructors, in general) it can't use global jsids.  Even the existing s_length_id usage we had was a tad suspect...

For constructor and prototype, I wonder whether we can add some jsfriendapi for getting the ids off cx->names() like js::LinkConstructorAndPrototype does.

>+++ b/dom/bindings/Codegen.py
> class CGDefineDOMInterfaceMethod(CGAbstractMethod):
>+  if (id != s_constructor_id) {

Why is that check needed?  Please document at the very least, but I can't tell why it's here.

>+    for (unsigned slot = 2; slot < JSCLASS_RESERVED_SLOTS(&InterfaceObjectClass.mBase); ++slot) {

s/2/DOM_INTERFACE_SLOTS_BASE/ ?

> class CGStaticMethod(CGAbstractStaticBindingMethod):
>+    def __init__(self, descriptor, method, name=None):

That "name" argument seems to be unused...  Why is this change needed?

>@@ -6336,16 +6374,19 @@ class CGDescriptor(CGThing):
>+        for n in descriptor.interface.namedConstructors:
>+            cgThings.append(CGClassConstructor(descriptor, n,
>+                                               '_' + n.identifier.name))

Please document that the '_' + n.identifier.name bit needs to match what we do in CGNamedConstructors (and document there that it needs to match the code here).

>+++ b/dom/bindings/parser/WebIDL.py
>     def resolveIdentifierConflict(self, scope, identifier, originalObject, newObject):

Why hoist the method bits up to IDLScope from IDLInterface?  Worth explaining at least as part of the commit message.  Is this because we resolve the constructor name on parentScope?  But why do we do that?  Is the idea to detect two different interfaces defining NamedConstructors with the same name?  If so we should document it clearly and make sure the spec forbids that...

>@@ -794,16 +797,49 @@ class IDLInterface(IDLObjectWithScope):
>+            elif identifier == "NamedConstructor":

So I'm not that happy with the copy/paste here.  Seems like the differences from "Constructor" are:

1)  The hasValue() check.
2)  The scope passed to resolve(), but see above.
3)  The "make sure we don't have conflicts with another interface" bits.

Would it be more or less ugly to combine the two cases and branch as needed?

r=me with the above nits (especially the jsid bit) addressed.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So I just checked with jorendorff, and he's totally fine with exposing some aspect of cx->names() via friend API, for what it's worth.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 715243 [details] [diff] [review]
Use NamedConstructor for Image() and Option() v1

The IDL for HTMLImageElement now uses optional arguments, not overloads.  Would like to see the patch updated to that.

For that matter, should Option do that too?

>+++ b/layout/build/nsLayoutModule.cpp

Do we still need the contract/cid stuff at all for image and option?

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: Add codegen support for NamedConstructor v1.1

Took care of the comments, feel free to look it over once more.

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: Use NamedConstructor for Image() and Option() v2

I'll open a spec bug for Option(...)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 716704 [details] [diff] [review]
Add codegen support for NamedConstructor v1.1

Looks good.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 716705 [details] [diff] [review]
Use NamedConstructor for Image() and Option() v2

>+++ b/content/html/content/src/HTMLImageElement.cpp
>+    aError = img->SetIntAttr(nsGkAtoms::width,
>+                             static_cast<int32_t>(aWidth.Value()));

Or:

  img->SetHTMLUnsignedIntAttr(nsGkAtoms::width, aWidth.Value(), aError);

and same for height.

>+++ b/content/html/content/src/HTMLOptionElement.cpp
>+      aError = option->SetAttr(kNameSpaceID_None, nsGkAtoms::value,
>+                               aValue.Value(), false);

Please document that this is not using SetHTMLAttr because you want to be able to pass aNotify == false?

>+          aError = option->SetAttr(kNameSpaceID_None, nsGkAtoms::selected,
>+                                   EmptyString(), false);

Likewise.

>+          aError = option->SetSelected(aSelected.Value());

  option->SetSelected(aSelected.Value(), aError);

>+++ b/layout/build/nsLayoutModule.cpp
>+  { "@mozilla.org/content/element/html;1?name=img", &kNS_HTMLIMAGEELEMENT_CID },
>+  { "@mozilla.org/content/element/html;1?name=option", &kNS_HTMLOPTIONELEMENT_CID },

Again, do we still need these? If not, we could simplify the NS_New methods for these classes, right?

r=me either way; I'm ok with a followup for removing those contracts and classids.

Comment 16

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f03d15b1ef9
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0ef7c57f220

I removed js/src/tests/js1_5/extensions/regress-407019.js, it was testing something XPConnect-specific for Option(...).

Comment 17

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/5f03d15b1ef9
https://hg.mozilla.org/mozilla-central/rev/d0ef7c57f220