Boris Zbarsky [:bzbarsky] Assignee
	Description • 12 years ago

IDL:

  dictionary WebGLContextEventInit : EventInit {
    DOMString statusMessage;
  };

Parser says:

  File "other-licenses/ply/ply/yacc.py", line 971, in parseopt_notrack
    p.callable(pslice)
  File "dom/bindings/parser/WebIDL.py", line 1831, in p_Definition
    assert p[1] # We might not have implemented something ...
  AssertionError

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 12 years ago

Note that it's possible that EventInit is not declared anywhere useful as far as the parser is concerned; in fact that's somewhat likely.  So maybe this is not exactly an issue with dictionaries....  hard to tell.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 12 years ago

As you noticed, there's no return value support for dictionaries so far.  A large part of this is that I haven't come up with a satisfying way of doing it.  To quote my current commit message for a WIP on those:

  This implementation is somewhat suboptimal, insofar as the types of arguments
  and return values don't match, since the dictionary's members have the types of
  arguments.  But in practice for the things we support inside dictionaries at
  the moment this works fine.

  Another issue is that callees MUST fill in the members of a dictionary that
  have default values.  The binding does not handle this for them.  We could make
  this safer by having all binding members, including the ones with default
  values, as Optional<>, but that makes the consumer code for using dictionary
  arguments (which should be the common case) more complicated.

Peter suggested that this last be handled via the binding pre-initializing the members with default values.  I can do that, at the cost of a bunch of extra complexity in the codegen (e.g. right now we have no hook for running possibly-fallible initialization code on the retval, so I would need to add that), but it's not quite clear what the point of having dictionary return values is in the first place, so I'm going to hold off on doing the work until we have a real need for it.  Note that the one consumer of dictionary retvals we have, WebGL, can't actually use them for real until we kill off the XPIDL bindings for the webgl context, so this is not exactly urgent.

I'll post the current (totally broken, since it's in mid-modification; it's missing that hook I talked about for one thing) code for retval dictionaries, for posterity.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 8 • 12 years ago

Comment on attachment 631421 [details] [diff] [review]
part 3.  Implement codegen for dictionary arguments.   implementation option would be to put all the dictionaries in a single

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

::: dom/bindings/BindingUtils.h
@@ +569,5 @@
>           NULL;
>  }
>  
> +static inline bool
> +InternJSString(JSContext* cx, jsid& id, const char* chars)

jsid*?

@@ +571,5 @@
>  
> +static inline bool
> +InternJSString(JSContext* cx, jsid& id, const char* chars)
> +{
> +  if (JSString *str = ::JS_InternString(cx, chars)) {

No ::, please, and * to the left

::: dom/bindings/Codegen.py
@@ +1723,5 @@
> +        # will come from the Optional or our container.
> +        mutableTypeName = declType
> +        if not isOptional and not isMember:
> +            declType = CGWrapper(declType, pre="const ")
> +            selfRef = "const_cast<%s&>(%s)" % (typeName, selfRef)

const_cast :/

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 12 years ago

> jsid*?

I considered it... it doesn't really matter here, I think.

> const_cast :/

Yes.  Feel free to come up with a better setup.  Too bad C++ has no generic "cast to const" thing.  :(

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 10 • 12 years ago

Comment on attachment 631418 [details] [diff] [review]
part 1.  Add support for dictionaries in the WebIDL parser.

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

::: dom/bindings/parser/WebIDL.py
@@ +611,5 @@
> +
> +        if self.parent:
> +            assert isinstance(self.parent, IDLIdentifierPlaceholder)
> +            self.parent = self.parent.finish(scope)
> +            assert isinstance(self.parent, IDLDictionary)

Don't make this an assertion.  Throw a real error if this check fails.

Something like:

interface Foo {
};

dictionary Bar : Foo {
};

will parse, and this is the first chance we have to reject it.  I want to keep assertions solely for parser bugs or things that aren't implemented.

@@ +619,5 @@
> +
> +        for member in self.members:
> +            member.resolve(self)
> +        # Members of a dictionary are sorted in lexicographic order
> +        self.members.sort(cmp=cmp, key=lambda x: x.identifier.name)

Nit: newline after the loop.

@@ +625,5 @@
> +        inheritedMembers = []
> +        ancestor = self.parent
> +        while ancestor:
> +            inheritedMembers.extend(ancestor.members)
> +            ancestor = ancestor.parent

This may seem dumb, but I think we should handle the following case:

dictionary A : B {
};

dictionary B : A {
};

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 12 years ago

> Don't make this an assertion.  Throw a real error if this check fails.

Done.  This:

        parser.parse("""
          interface Iface {};
          dictionary Dict : Iface {
            long prop;
          };
        """)

now gives this output:

  error: Dictionary Dict has parent that is not a dictionary, <unknown> line 3:28
            dictionary Dict : Iface {
                              ^
  <unknown> line 2:10
            interface Iface {};
            ^

> Nit: newline after the loop.

Done.

> This may seem dumb, but I think we should handle the following case:

Done.

	Peter Van der Beken [:peterv]
	Comment 12 • 12 years ago

Comment on attachment 631421 [details] [diff] [review]
part 3.  Implement codegen for dictionary arguments.   implementation option would be to put all the dictionaries in a single

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

::: dom/bindings/Codegen.py
@@ +361,5 @@
>                          if typeDesc is not None:
>                              implementationIncludes.add(typeDesc.headerFile)
> +                            bindingHeaders.add(self.getDeclarationFilename(typeDesc.interface))
> +                elif t.unroll().isDictionary():
> +                    bindingHeaders.add(self.getDeclarationFilename(t.inner))

This doesn't need to .unroll()?

@@ +3429,5 @@
> +        else:
> +            inheritance = ""
> +        memberDecls = [ "  %s %s;" %
> +                        (self.getMemberType(m), m.identifier.name)
> +                        for m in d.members ]

I don't think we add spaces after [ and before ] elsewhere in this file (there's a couple of these in the patch).

@@ +3444,5 @@
> +                "  static bool InitIds(JSContext* cx);\n"
> +                "  static bool initedIds;\n" +
> +                "\n".join(["  static jsid " +
> +                           self.makeIdName(m.identifier.name) + ";" for
> +                           m in d.members]) + "\n"

I don't think you need the square brackets here (and iiuc it would be faster since you wouldn't create a list but use an iterator).

@@ +3509,5 @@
> +        suffix = "Workers" if self.workers else ""
> +        return dictionary.identifier.name + suffix
> +
> +    def getMemberType(self, member):
> +        # Fake a descriptorProvider

Can you add a comment here that this will fail for interfaces (and maybe assert that member.type is not an interface).

@@ +3511,5 @@
> +
> +    def getMemberType(self, member):
> +        # Fake a descriptorProvider
> +        (templateBody, declType,
> +         holderType, dealWithOptional) = getJSToNativeConversionTemplate(

Can we call this in init(...) since you do it twice (in getMemberType and in getMemberConversion).

@@ +3677,5 @@
> +            reSortedDictionaries.extend(toMove)
> +
> +        dictionaries = reSortedDictionaries
> +        cgthings.extend([ CGDictionary(d, workers=True) for d in dictionaries ])
> +        cgthings.extend([ CGDictionary(d, workers=False) for d in dictionaries ])

We could pass in descriptors here? File a followup anyway to make interface members work.

@@ +3684,5 @@
>          cgthings.extend([CGDescriptor(x) for x in descriptors])
> +
> +        # And make sure we have a newline at the end
> +        cgthings.append(CGGeneric("\n"))
> +        curr = CGList(cgthings, "\n\n")

Shouldn't this be

  curr = CGWrapper(CGList(cgthings, "\n\n"), post="\n")
?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 12 years ago

> This doesn't need to .unroll()?

It does!  Good catch.

> I don't think we add spaces after [ and before ] elsewhere in this file 

OK.  Fixed.

> I don't think you need the square brackets here 

Fixed, but we have lots of that pattern in this file. :(

> Can you add a comment here that this will fail for interfaces (and maybe assert that
> member.type is not an interface).

Done.

> Can we call this in init(...) 

Done.

> We could pass in descriptors here?

Not easily, but we could pass in the config and refactor it a tad.  I'll do that in bug 763911.

> Shouldn't this be

Changed to that.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/21975db074d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/d82140dbb2a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/84536fdda9b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/16f1a804057c

	Matt Brubeck (:mbrubeck)
	Comment 15 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/21975db074d8
https://hg.mozilla.org/mozilla-central/rev/d82140dbb2a5
https://hg.mozilla.org/mozilla-central/rev/84536fdda9b7
https://hg.mozilla.org/mozilla-central/rev/16f1a804057c