Need WebIDL binding support for dictionaries

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla16
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

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
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.
Assignee: khuey → bzbarsky
Summary: Need WebIDL parser support for dictionaries → Need WebIDL binding support for dictionaries
Created attachment 631418 [details] [diff] [review]
part 1.  Add support for dictionaries in the WebIDL parser.
Attachment #631418 - Flags: review?(khuey)
Created attachment 631420 [details] [diff] [review]
part 2.  Rename isSequenceMember to isMember, since it will apply to both sequences and dictionaries.
Attachment #631420 - Flags: review?(peterv)
Created attachment 631421 [details] [diff] [review]
part 3.  Implement codegen for dictionary arguments.   implementation option would be to put all the dictionaries in a single
Attachment #631421 - Flags: review?(peterv)
Created attachment 631422 [details] [diff] [review]
part 4.  Allow strings inside sequences and dictionaries.
Attachment #631422 - Flags: review?(peterv)
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.
Created attachment 631429 [details] [diff] [review]
dictionary retval hackery
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 :/
> 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.  :(
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 {
};
Attachment #631418 - Flags: review?(khuey) → review+
> 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.
Attachment #631420 - Flags: review?(peterv) → review+
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")
?
Attachment #631421 - Flags: review?(peterv) → review+
Attachment #631422 - Flags: review?(peterv) → review+
> 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.
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
Flags: in-testsuite+
Target Milestone: --- → mozilla16
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 764698
You need to log in before you can comment on or make changes to this bug.