854490 - Support constructors for WebIDL dictionaries

Status: RESOLVED INVALID

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

Description

[Jet Villegas (inactive)]

WebIDL dictionaries don't have constructors. This bug add support for that:

[Constructor]
dictionary DOMPoint {
  float x = 0;
  float y = 0;
};

Comment 1

[Jet Villegas (inactive)]

This comment belongs here...

(Cameron McCormack (:heycam) wrote on bug 850805)
> I've added this now:
> 
> http://dev.w3.org/2006/webapi/WebIDL/#Constructor
> http://dev.w3.org/2006/webapi/WebIDL/#es-dictionary-constructors
> 
> http://dev.w3.org/cvsweb/2006/webapi/WebIDL/Overview.xml.diff?r1=1.617;r2=1.
> 618;f=h

Comment 2

[Jet Villegas (inactive)]

Attachment: Adds WebIDL binding support

Add WebIDL binding support for dictionaries with [Constructor] and [NamedConstructor] attributes.

Comment 3

[Cameron McCormack (:heycam)]

Note that in the spec I've allowed [Constructor] on dictionaries but not [NamedConstructor], but only because it didn't seem like it was necessary.  Let me know if this is in fact needed.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Note that the patch just adds support to the parser and data model.  Actually doing the codegen for these things and exposing them on the window are separate things...

I don't think we really need NamedConstructor for dictionaries so far.

Comment 5

[Jet Villegas (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #4)
> Note that the patch just adds support to the parser and data model. 
> Actually doing the codegen for these things and exposing them on the window
> are separate things...

Right. Still working on that bit.

> I don't think we really need NamedConstructor for dictionaries so far.

NamedConstructor is how I'm aliasing WebKitPoint() to DOMPoint() in bug 850805. Maybe there's a smarter way to do that?

Comment 6

[Boris Zbarsky [:bzbarsky]]

I think you could just duplicate the dictionary in the IDL, just as easily.  Not sure whether that's better than building out infrastructure for named constructors here or not....

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 733149 [details] [diff] [review]
Adds WebIDL binding support

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

I'd prefer that we put the Constructor/NamedConstructor handling logic in a single function that IDLInterface/IDLDictionary can share.

::: dom/bindings/parser/WebIDL.py
@@ +1128,5 @@
> +
> +                args = attr.args() if attr.hasArgs() else []
> +
> +                retType = IDLWrapperType(self.location, self)
> +                

extraneous whitespace!

Comment 8

[Jet Villegas (inactive)]

Attachment: DOM binding decl for dictionary constructor

Comment 9

[Jet Villegas (inactive)]

Attachment: DOM binding impl for dictionary constructor

I think this is how the bindings should look like for dictionary constructors based on earlier feedback from bz & khuey. Before I hack up the codegen.py for this, I'd like some feedback on the following items in this file:

line 190: contents of DOMIfaceAndProtoJSClass InterfaceObjectClass 
line 216: contents of DOMIfaceAndProtoJSClass PrototypeClass 
line 261: parameters to dom::CreateInterfaceObjects
line 281: JSCLASS_HAS_RESERVED_SLOTS(?)

Thanks!

Comment 10

[Boris Zbarsky [:bzbarsky]]

We shouldn't have a PrototypeClass at all here, or a prototype object.

Not sure whether DOMPoint.prototype should be undefined or Object.prototype.  Perhaps the latter, since that's what we'd do in practice.

Comment 11

[Jet Villegas (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #10)
> We shouldn't have a PrototypeClass at all here, or a prototype object.

OK.

> Not sure whether DOMPoint.prototype should be undefined or Object.prototype.
> Perhaps the latter, since that's what we'd do in practice.

It looks like we use 'undefined' in practice, or at least that's what "nullptr" seems to do:

        if needInterfacePrototypeObject:
            protoClass = "&PrototypeClass.mBase"
            protoCache = "&protoAndIfaceArray[prototypes::id::%s]" % self.descriptor.name
        else:
            protoClass = "nullptr"
            protoCache = "nullptr"

Comment 12

[Boris Zbarsky [:bzbarsky]]

> It looks like we use 'undefined' in practice

For interfaces with no prototype, yes.  And I'm not sure that's the right thing to do either, fwiw; it means we end up with Function.prototype as the .prototype!

Comment 13

[Jet Villegas (inactive)]

Attachment: DOM binding decl for dictionary constructor

This header file was auto-generated by the codegen changes in this bug fix.

Comment 14

[Jet Villegas (inactive)]

Attachment: DOM binding impl for dictionary constructor

This cpp file was auto-generated by the codegen changes in this bug fix.

Comment 15

[Jet Villegas (inactive)]

Attachment: Adds WebIDL binding support

Modifies the python code to generate constructor bindings for JS dictionary types.

Comment 16

[Jet Villegas (inactive)]

/* This is the WebIDL source for attachment 744449 [details] and attachment 744450 [details] */

[Constructor(optional float x = 0, optional float y = 0),Pref="layout.css.dompoint.enabled"]
dictionary DOMPoint {
  float x = 0;
  float y = 0;
};

Comment 17

[Boris Zbarsky [:bzbarsky]]

Would be good to at least add some codegen and webidl parser tests for this stuff.

Comment 18

[Jet Villegas (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #17)
> Would be good to at least add some codegen and webidl parser tests for this
> stuff.

Yes! this needs proper tests. Working on it...

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 744453 [details] [diff] [review]
Adds WebIDL binding support

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

you have whitespace at the end of lines all over the place.

This looks pretty good.  I'd like Boris to do the final signoff though.

::: dom/bindings/parser/WebIDL.py
@@ +484,5 @@
>  
>      def _getDependentObjects(self):
>          return set()
>  
> +class IDLConstructable():

class IDLConstructable(object):

Add a doc comment explaining what this is and that it's intended to be used as a mixin for things inheriting from IDLObjectWithScope.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 744453 [details] [diff] [review]
Adds WebIDL binding support

Jet, do you want to merge on top of bug 873417 before I review the details?

For the overall approach, I think the changes to WebIDL.py look fine.  I'm a bit concerned about the change to CGCallGenerator to assume that the descriptorProvider is in fact a descriptor.  If that's the case, we should rename it and document which thing it's supposed to be a descriptor for.  

>+            builder.add(d.nativeType,d.interface.isDictionary)

isDictionary(), most importantly.  But also, isStruct=d.interface.isDictionary(), please.

It bothers me conceptually to have descriptor.interface not be an interface, but I'm not going to make you rename it to something else... though maybe as a followup we should.

It looks like for now we only support dictionary members that have a default value and can be assigned with '=', right?

The biggest correctness problem here is that we need to root the dictionary.  So ideally the codegen would look more like this:

  RootedDictionary<WhateverType> result(cx);
  result.Init(args);

or something along those lines.  Passing the args directly to the RootedDictionary constructor is hard, unfortunately, since we don't codegen that constructor....

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I think we decided this wasn't going to happen, no?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Yes. It's gone from the spec.