Last Comment Bug 742153 - Need WebIDL binding support for dictionaries
: Need WebIDL binding support for dictionaries
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 764698
Blocks: ParisBindings
  Show dependency treegraph
 
Reported: 2012-04-03 19:50 PDT by Boris Zbarsky [:bz]
Modified: 2012-06-13 20:55 PDT (History)
5 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1. Add support for dictionaries in the WebIDL parser. (16.47 KB, patch)
2012-06-08 09:09 PDT, Boris Zbarsky [:bz]
khuey: review+
Details | Diff | Review
part 2. Rename isSequenceMember to isMember, since it will apply to both sequences and dictionaries. (8.88 KB, patch)
2012-06-08 09:09 PDT, Boris Zbarsky [:bz]
peterv: review+
Details | Diff | Review
part 3. Implement codegen for dictionary arguments. implementation option would be to put all the dictionaries in a single (27.60 KB, patch)
2012-06-08 09:10 PDT, Boris Zbarsky [:bz]
peterv: review+
Details | Diff | Review
part 4. Allow strings inside sequences and dictionaries. (5.84 KB, patch)
2012-06-08 09:10 PDT, Boris Zbarsky [:bz]
peterv: review+
Details | Diff | Review
dictionary retval hackery (17.59 KB, patch)
2012-06-08 09:18 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review

Description Boris Zbarsky [:bz] 2012-04-03 19:50:23 PDT
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
Comment 1 Boris Zbarsky [:bz] 2012-04-03 20:16:56 PDT
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.
Comment 2 Boris Zbarsky [:bz] 2012-06-08 09:09:29 PDT
Created attachment 631418 [details] [diff] [review]
part 1.  Add support for dictionaries in the WebIDL parser.
Comment 3 Boris Zbarsky [:bz] 2012-06-08 09:09:53 PDT
Created attachment 631420 [details] [diff] [review]
part 2.  Rename isSequenceMember to isMember, since it will apply to both sequences and dictionaries.
Comment 4 Boris Zbarsky [:bz] 2012-06-08 09:10:20 PDT
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
Comment 5 Boris Zbarsky [:bz] 2012-06-08 09:10:52 PDT
Created attachment 631422 [details] [diff] [review]
part 4.  Allow strings inside sequences and dictionaries.
Comment 6 Boris Zbarsky [:bz] 2012-06-08 09:16:58 PDT
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.
Comment 7 Boris Zbarsky [:bz] 2012-06-08 09:18:18 PDT
Created attachment 631429 [details] [diff] [review]
dictionary retval hackery
Comment 8 :Ms2ger 2012-06-08 09:27:24 PDT
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 :/
Comment 9 Boris Zbarsky [:bz] 2012-06-08 10:17:53 PDT
> 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 10 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-06-09 14:09:58 PDT
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 {
};
Comment 11 Boris Zbarsky [:bz] 2012-06-11 15:11:16 PDT
> 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.
Comment 12 Peter Van der Beken [:peterv] 2012-06-12 02:31:44 PDT
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")
?
Comment 13 Boris Zbarsky [:bz] 2012-06-12 07:19:46 PDT
> 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.

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