458936 - Replace binary xpidl xpt output with a python version hooked up to xpidl.py

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benjamin Smedberg]

We currently are using the binary xpidl tool to produce XPT output. We should port this logic to python and get rid of the binary XPIDL parser for good.

Relevant links:
existing binary XPT output code:
http://mxr.mozilla.org/mozilla-central/source/xpcom/typelib/xpidl/xpidl_typelib.c

new python XPIDL parser core:
http://mxr.mozilla.org/mozilla-central/source/xpcom/idl-parser/xpidl.py

code to produce C++ headers from the python core:
http://mxr.mozilla.org/mozilla-central/source/xpcom/idl-parser/header.py

The magic here is, I'm afraid, that the XPT format is under-documented, so you get to have the fun of reverse-engineering parts of it. The available docs are here:
http://www.mozilla.org/scriptable/typelib_file.html

Comment 1

[(not currently active) Ted Mielczarek]

Needs to be able to both generate and read the xpt files, so it can replace xpidl and xpt_link. We should create some unit tests before even starting this. :)

Comment 2

[Benjamin Smedberg]

I don't think it's necessary to replace xpt_link in this bug. xpt_link doesn't depend on glib/libIDL, as far as I know; even though it's still a damnable host program ;-)

Comment 3

[Dirkjan Ochtman (:djc)]

I'll probably start off writing tests anyway.

Comment 4

[Dirkjan Ochtman (:djc)]

What Python version are we writing for? Is 2.4 good enough, or do we need 2.3 compatibility? bsmedberg, getName/setName is really not very Pythonic (from xpidl.py)...

Ted mentioned we like 2.3 support in the build system.

Comment 5

[Benjamin Smedberg]

We want 2.3 compat unless there's some really serious reason we need a later version.

Comment 6

[Zack Weinberg (:zwol)]

I started looking into this, by writing a functional equivalent of xpt_dump in Python.  (It does not produce the same output format.)  I will attach it when I get back to the computer it's on, but here are some tidbits about the file format that might be useful:

 * http://www.mozilla.org/scriptable/typelib_file.html says that the
   interface_directory pointer in the file header is a zero-based offset
   from the beginning of the file.  THIS IS WRONG.  It is a *one*-based
   offset from the beginning of the file.  The data_pool pointer, however,
   is zero-based.

 * parent_interface_index zero is documented to mean nsISupports, but the
   existing tools actually use it to mean "this interface has no parent"
   (e.g. in the .xpt file for nsISupports itself).  nsISupports gets a regular
   entry in the interface directory of each file that needs to refer to it.

 * Type tags #13 (void) and #14 (uuid) can appear without the pointer flag.
   Also, #14 is not necessarily an IID.  (xpt_dump thinks #14 requires the
   pointer flag, but it's wrong.)

 * Reserved type tags #23, #24, #25 have been assigned meaning; if
   xpt_dump is to be believed, they are "UTF8String", "CString", and
   "AString" respectively, require the pointer flag, and use 
   the SimpleTypeDescriptor variant.

 * The first reserved ParamDescriptor flag bit (value 0x04, treating the
   entire set of flags as a byte quantity) has been assigned; it means
   "optional".

That's everything I can remember.

I'd be happy to write an update for typelib_file.html provided I can bug someone about which of the above were changes in format 1.2 and which were always that way just misdocumented.

Comment 7

[Zack Weinberg (:zwol)]

Oh, also, if you're going for byte-for-byte compatibility with the existing generators, be aware that they do *not* unify duplicate strings.

Comment 8

[Benjamin Smedberg]

Well, byte-for-byte compatibility seemed like the best way to verify that untested code was behaving sanely. If you want to use an alternative strategy (identical xpt_dump output, for example, or something similar), please feel free to propose something!

Unless the XPT version was changed after Firefox 1 (which I seriously doubt) then I don't think we need to worry about older versions at all.

Comment 9

[Zack Weinberg (:zwol)]

I'm definitely down with byte-for-byte compatibility as a condition for switching over.  I just thought I'd mention the string thing as something that might surprise, and perhaps also as something that should be changed after the switchover.

I'm only interested in <1.2 versions for the documentation's sake, but perhaps it would be clearer to just not bother documenting what old versions were like.

Comment 10

[Benjamin Smedberg]

Unless it's easy to find out, I think just documenting the current version is quite reasonable and probably clearer.

Comment 11

[Zack Weinberg (:zwol)]

Attachment: python xpt dumper

Here's my python xpt dumper.  It's probably not great as a basis for an xpt *writer*, but it may still be useful for reference.  I made a start on a more structured xpt library but didn't get far enough for it to be interesting, I think.

Comment 13

[(not currently active) Ted Mielczarek]

The URL is my Python XPT reading/writing library. It can properly round-trip all the xpt files produced in a Firefox build, and its xpt_dump output exactly matches the binary xpt_dump.

Comment 14

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

I have a patch that is correct enough to start the browser.  Tracking down the remaining issues now.

Comment 15

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

Attachment: typelib.py

Throwing this at try now, but I think it's good enough for a review.

Comment 16

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

Attachment: typelib.py

Fix a bug with jsvals being weird.

Comment 17

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

Attachment: typelib.py

Ugh, hg being dumb.

Comment 18

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

Attachment: typelib.py

And this handles size_is with strings.  This passes tests locally for me, throwing it at try to verify.

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 551755 [details] [diff] [review]
typelib.py

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

::: config/rules.mk
@@ +1553,5 @@
> +	$(PYTHON_PATH) \
> +	  -I$(topsrcdir)/other-licenses/ply \
> +	  -I$(topsrcdir)/xpcom/idl-parser \
> +	  -I$(topsrcdir)/xpcom/typelib/xpt/tools \
> +	  $(topsrcdir)/xpcom/idl-parser/typelib.py --cachedir=$(topsrcdir)/xpcom/idl-parser $(XPIDL_FLAGS) $(_VPATH_SRCS) -d $(MDDEPDIR)/$(@F).pp -o $@

You should file a followup on having a single pass invocation that generates both headers and typelibs in one shot. (Also we could probably make the script handle multiple IDL files in one shot, which would be even nicer. It could cache the parse tree for things like nsISupports which would probably save some time.)

::: xpcom/idl-parser/typelib.py
@@ +69,5 @@
> +    'cstring':            xpt.Type.Tags.CString,
> +    'jsval':              xpt.Type.Tags.jsval
> +}
> +
> +# XXXkhuey dipper types should go away

File a bug on this and list the bug number?

@@ +73,5 @@
> +# XXXkhuey dipper types should go away
> +def isDipperType(type):
> +    return type == xpt.Type.Tags.DOMString or type == xpt.Type.Tags.AString or type == xpt.Type.Tags.CString or type == xpt.Type.Tags.UTF8String
> +
> +def write_interface(iface, ifaces):

I'd call this something other than "write_interface", since it doesn't write anything, it just returns an xpt.Interface object. build_interface? populate_interface?

@@ +84,5 @@
> +        if isinstance(type, xpidl.Builtin):
> +            tag = TypeMap[type.name]
> +            return xpt.SimpleType(tag,
> +                                  pointer=(tag == xpt.Type.Tags.char_ptr or tag == xpt.Type.Tags.wchar_t_ptr),
> +                                  #XXXkhuey unique_pointer is completely unused

File a bug on removing it and mention the bug number here. Also, unique_pointer defaults to False, so you don't have to specify it here at all.

@@ +90,5 @@
> +                                  reference=False)
> +
> +        if isinstance(type, xpidl.Array):
> +            return xpt.ArrayType(get_type(type.type, calltype), size_is,
> +                                 #XXXkhuey length_is is not actually used,

Should probably say "length_is duplicates size_is", and file a bug on removing it and mention the bug number here...

@@ +97,5 @@
> +        if isinstance(type, xpidl.Interface) or isinstance(type, xpidl.Forward):
> +            xptiface = None
> +            for i in ifaces:
> +                if i.name == type.name:
> +                    xptiface = i

We should probably just build a dict of interface name -> interface and use that instead.

@@ +101,5 @@
> +                    xptiface = i
> +
> +            if not xptiface:
> +                xptiface = xpt.Interface(name=type.name)
> +                ifaces.append(xptiface)

Hrm. If you have a forward declaration of an interface, used in an interface declaration, and then an actual declaration of the interface later in the same file, this would probably produce a malformed typelib. (You'd put an unresolved and a resolved interface descriptor in the same file.) I doubt that ever happens in practice, but we should probably try to avoid it.

@@ +126,5 @@
> +                                      unique_pointer=False,
> +                                      reference=False)
> +
> +        print type(type)
> +        raise Exception("fooey")

Presumably you want this to say something more useful. :)

@@ +148,5 @@
> +        type = get_type(m.realtype, 'out')
> +        if isDipperType(type.tag):
> +            # NB: The retval bit needs to be set here, contrary to what the
> +            # xpt spec says.
> +            return xpt.Param(type, True, False, True, dipper=True)

You should probably pass all these args by keyword. I wrote that code and I have no idea what the args are here.

@@ +151,5 @@
> +            # xpt spec says.
> +            return xpt.Param(type, True, False, True, dipper=True)
> +        return xpt.Param(type, False, True, True)
> +
> +    def get_attr_param(a, get=False, set=False):

You might want to name these "getter" and "setter" for consistency with other code.

@@ +155,5 @@
> +    def get_attr_param(a, get=False, set=False):
> +        if not (get or set):
> +            raise Exception("Attribute param must be for a getter or a setter!")
> +
> +        type = get_type(a.realtype, get == True and 'out' or 'in')

"get == True" is redundant, just say "get". Also, Python has a true ternary expression, you could write this as:
'out' if get else 'in'
but some people don't like that syntax, so it's up to you.

@@ +171,5 @@
> +
> +    consts = []
> +    methods = []
> +
> +    def write_const(c):

Again, "write" sounds like a misnomer.

@@ +178,5 @@
> +    def write_method(m):
> +        params = []
> +
> +        def get_param(p):
> +            def findattr(p, attr):

This could be ...more usefully named, I think. "find_referenced_param" or something? A docstring comment wouldn't hurt either.

@@ +179,5 @@
> +        params = []
> +
> +        def get_param(p):
> +            def findattr(p, attr):
> +                if hasattr(p, attr) and getattr(p, attr):

Why the "and getattr"? To avoid None values? Seems a little weird.

@@ +182,5 @@
> +            def findattr(p, attr):
> +                if hasattr(p, attr) and getattr(p, attr):
> +                    for param in m.params:
> +                        if param.name == getattr(p, attr):
> +                            return m.params.index(param)

You could do:
for i, param in enumerate(m.params):
and just return i.

@@ +206,5 @@
> +            params.append(get_retval_param(m))
> +
> +        methods.append(xpt.Method(m.name, get_result_param(m), params, False,
> +                                  False, m.notxpcom, False, m.noscript,
> +                                  m.optional_argc, m.implicit_jscontext))

Again, can you use keyword args here, at least when you're just passing boolean literals?

@@ +238,5 @@
> +    parent = None
> +    if iface.base:
> +        for i in ifaces:
> +            if i.name == iface.base:
> +                parent = i

Really ought to be a dict lookup. Does idl.resolve not set these to the actual interface types for you? (If not we should probably make it do that.)

@@ +241,5 @@
> +            if i.name == iface.base:
> +                parent = i
> +        if not parent:
> +            parent = xpt.Interface(name=iface.base)
> +            ifaces.append(parent)

This has the same potential bug as I described earlier.

@@ +245,5 @@
> +            ifaces.append(parent)
> +
> +    return xpt.Interface(iface.name, iface.attributes.uuid, methods=methods,
> +                         constants=consts, resolved=True, parent=parent,
> +                         scriptable=not iface.attributes.noscript,

Shouldn't this technically be "iface.attributes.scriptable and not iface.attributes.noscript". I don't know why we support both of those on an interface, seems dumb.

@@ +262,5 @@
> +    typelib = xpt.Typelib(interfaces=ifaces)
> +    typelib.writefd(fd)
> +
> +if __name__ == '__main__':
> +    from optparse import OptionParser

How about you stick this all in a "def main()" and just call main() here, in case we get an opportunity to ditch pythonpath.py and still want to use this as a native command?

@@ +285,5 @@
> +        print >>sys.stderr, "-d requires -o"
> +        sys.exit(1)
> +
> +    if options.outfile is not None:
> +        outfd = open(options.outfile, 'wb')

Is there a reason you open the file here and not in write_typelib? You'd only have to pass down the filename if you did that.

@@ +286,5 @@
> +        sys.exit(1)
> +
> +    if options.outfile is not None:
> +        outfd = open(options.outfile, 'wb')
> +        closeoutfd = True

Feels like you just want to "with open(outfile, 'wb') as outfd". (You'll need from __future__ import with_statement to use that reliably.)

@@ +299,5 @@
> +    if closeoutfd:
> +        outfd.close()
> +
> +    if options.depfile is not None:
> +        depfd = open(options.depfile, 'w')

with open(options.depfile, 'w') as depfd: (Also it's a little weird to name these variables "fd", since they're really file objects. You can get real file descriptors in Python with os.open.)

::: xpcom/typelib/xpt/tools/xpt.py
@@ +145,5 @@
>          self.pointer = pointer
>          self.unique_pointer = unique_pointer
>          self.reference = reference
> +        if reference and not pointer:
> +            raise Exception("If reference is True pointer must be True too")

Good catch.

@@ +645,5 @@
>          self.optargc = optargc
>          self.implicit_jscontext = implicit_jscontext
>          self.params = list(params)
> +        if result and not isinstance(result, Param):
> +            raise Exception("result must be a Param!")

Good catch here too. Want to add unit tests for these two cases to the test suite?

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 551856 [details] [diff] [review]
typelib.py

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

Overall this looks good. Any thoughts on a test suite? With slight refactoring of typelib.py you could make it take an IDL file and return the XPT object, and you could at least sanity check your mappings. The xpt.py tests already test round-tripping of data to and from a file, so that's reasonably well tested.

::: xpcom/idl-parser/typelib.py
@@ +84,5 @@
> +        if isinstance(type, xpidl.Builtin):
> +            if type.name == 'string' and size_is != None:
> +                  return xpt.StringWithSizeType(size_is, size_is)
> +            elif type.name == 'wstring' and size_is != None:
> +                  return xpt.WideStringWithSizeType(size_is, size_is)

I'd say this ought to be a lookup table, but there are only two entries anyway. :-/

Comment 21

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

(In reply to Ted Mielczarek [:ted, :luser] from comment #19)
> Comment on attachment 551755 [details] [diff] [review] [diff] [details] [review]
> typelib.py
> 
> Review of attachment 551755 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: config/rules.mk
> @@ +1553,5 @@
> > +	$(PYTHON_PATH) \
> > +	  -I$(topsrcdir)/other-licenses/ply \
> > +	  -I$(topsrcdir)/xpcom/idl-parser \
> > +	  -I$(topsrcdir)/xpcom/typelib/xpt/tools \
> > +	  $(topsrcdir)/xpcom/idl-parser/typelib.py --cachedir=$(topsrcdir)/xpcom/idl-parser $(XPIDL_FLAGS) $(_VPATH_SRCS) -d $(MDDEPDIR)/$(@F).pp -o $@
> 
> You should file a followup on having a single pass invocation that generates
> both headers and typelibs in one shot. (Also we could probably make the
> script handle multiple IDL files in one shot, which would be even nicer. It
> could cache the parse tree for things like nsISupports which would probably
> save some time.)

I think I'll just repurpose bug 330124.

> ::: xpcom/idl-parser/typelib.py
> @@ +69,5 @@
> > +    'cstring':            xpt.Type.Tags.CString,
> > +    'jsval':              xpt.Type.Tags.jsval
> > +}
> > +
> > +# XXXkhuey dipper types should go away
> 
> File a bug on this and list the bug number?

Bug 677784.

> @@ +73,5 @@
> > +# XXXkhuey dipper types should go away
> > +def isDipperType(type):
> > +    return type == xpt.Type.Tags.DOMString or type == xpt.Type.Tags.AString or type == xpt.Type.Tags.CString or type == xpt.Type.Tags.UTF8String
> > +
> > +def write_interface(iface, ifaces):
> 
> I'd call this something other than "write_interface", since it doesn't write
> anything, it just returns an xpt.Interface object. build_interface?
> populate_interface?

build_interface it is

> @@ +84,5 @@
> > +        if isinstance(type, xpidl.Builtin):
> > +            tag = TypeMap[type.name]
> > +            return xpt.SimpleType(tag,
> > +                                  pointer=(tag == xpt.Type.Tags.char_ptr or tag == xpt.Type.Tags.wchar_t_ptr),
> > +                                  #XXXkhuey unique_pointer is completely unused
> 
> File a bug on removing it and mention the bug number here. Also,
> unique_pointer defaults to False, so you don't have to specify it here at
> all.

Bug 677787.

> @@ +90,5 @@
> > +                                  reference=False)
> > +
> > +        if isinstance(type, xpidl.Array):
> > +            return xpt.ArrayType(get_type(type.type, calltype), size_is,
> > +                                 #XXXkhuey length_is is not actually used,
> 
> Should probably say "length_is duplicates size_is", and file a bug on
> removing it and mention the bug number here...

Bug 677788

> @@ +97,5 @@
> > +        if isinstance(type, xpidl.Interface) or isinstance(type, xpidl.Forward):
> > +            xptiface = None
> > +            for i in ifaces:
> > +                if i.name == type.name:
> > +                    xptiface = i
> 
> We should probably just build a dict of interface name -> interface and use
> that instead.

I chose not to do that because it we'd have to maintain the dict and the list.

> @@ +101,5 @@
> > +                    xptiface = i
> > +
> > +            if not xptiface:
> > +                xptiface = xpt.Interface(name=type.name)
> > +                ifaces.append(xptiface)
> 
> Hrm. If you have a forward declaration of an interface, used in an interface
> declaration, and then an actual declaration of the interface later in the
> same file, this would probably produce a malformed typelib. (You'd put an
> unresolved and a resolved interface descriptor in the same file.) I doubt
> that ever happens in practice, but we should probably try to avoid it.

It does happen in practice, and it's handled when XPTs get merged.

> @@ +126,5 @@
> > +                                      unique_pointer=False,
> > +                                      reference=False)
> > +
> > +        print type(type)
> > +        raise Exception("fooey")
> 
> Presumably you want this to say something more useful. :)

Ah, this was debugging code.  This should just be unreached now.

> @@ +148,5 @@
> > +        type = get_type(m.realtype, 'out')
> > +        if isDipperType(type.tag):
> > +            # NB: The retval bit needs to be set here, contrary to what the
> > +            # xpt spec says.
> > +            return xpt.Param(type, True, False, True, dipper=True)
> 
> You should probably pass all these args by keyword. I wrote that code and I
> have no idea what the args are here.

Heh, I thought the same when I was writing it.

> @@ +151,5 @@
> > +            # xpt spec says.
> > +            return xpt.Param(type, True, False, True, dipper=True)
> > +        return xpt.Param(type, False, True, True)
> > +
> > +    def get_attr_param(a, get=False, set=False):
> 
> You might want to name these "getter" and "setter" for consistency with
> other code.

Done.

> @@ +155,5 @@
> > +    def get_attr_param(a, get=False, set=False):
> > +        if not (get or set):
> > +            raise Exception("Attribute param must be for a getter or a setter!")
> > +
> > +        type = get_type(a.realtype, get == True and 'out' or 'in')
> 
> "get == True" is redundant, just say "get". Also, Python has a true ternary
> expression, you could write this as:
> 'out' if get else 'in'
> but some people don't like that syntax, so it's up to you.

I copied the style of the existing pyxpidl code here, and I think I'll leave it.  I've dropped the unnecessary comparison.

> @@ +171,5 @@
> > +
> > +    consts = []
> > +    methods = []
> > +
> > +    def write_const(c):
> 
> Again, "write" sounds like a misnomer.

Changed to build.

> @@ +178,5 @@
> > +    def write_method(m):
> > +        params = []
> > +
> > +        def get_param(p):
> > +            def findattr(p, attr):
> 
> This could be ...more usefully named, I think. "find_referenced_param" or
> something? A docstring comment wouldn't hurt either.

I've changed it to build_param to match the others.

> @@ +179,5 @@
> > +        params = []
> > +
> > +        def get_param(p):
> > +            def findattr(p, attr):
> > +                if hasattr(p, attr) and getattr(p, attr):
> 
> Why the "and getattr"? To avoid None values? Seems a little weird.

Yes.  That happens, no idea why.

> @@ +182,5 @@
> > +            def findattr(p, attr):
> > +                if hasattr(p, attr) and getattr(p, attr):
> > +                    for param in m.params:
> > +                        if param.name == getattr(p, attr):
> > +                            return m.params.index(param)
> 
> You could do:
> for i, param in enumerate(m.params):
> and just return i.

Neat

> @@ +206,5 @@
> > +            params.append(get_retval_param(m))
> > +
> > +        methods.append(xpt.Method(m.name, get_result_param(m), params, False,
> > +                                  False, m.notxpcom, False, m.noscript,
> > +                                  m.optional_argc, m.implicit_jscontext))
> 
> Again, can you use keyword args here, at least when you're just passing
> boolean literals?

Done.

> @@ +238,5 @@
> > +    parent = None
> > +    if iface.base:
> > +        for i in ifaces:
> > +            if i.name == iface.base:
> > +                parent = i
> 
> Really ought to be a dict lookup. Does idl.resolve not set these to the
> actual interface types for you? (If not we should probably make it do that.)
>
> @@ +241,5 @@
> > +            if i.name == iface.base:
> > +                parent = i
> > +        if not parent:
> > +            parent = xpt.Interface(name=iface.base)
> > +            ifaces.append(parent)
> 
> This has the same potential bug as I described earlier.

And our code handles it fine.

> @@ +245,5 @@
> > +            ifaces.append(parent)
> > +
> > +    return xpt.Interface(iface.name, iface.attributes.uuid, methods=methods,
> > +                         constants=consts, resolved=True, parent=parent,
> > +                         scriptable=not iface.attributes.noscript,
> 
> Shouldn't this technically be "iface.attributes.scriptable and not
> iface.attributes.noscript". I don't know why we support both of those on an
> interface, seems dumb.

Yes.  I didn't know we had both!

> @@ +262,5 @@
> > +    typelib = xpt.Typelib(interfaces=ifaces)
> > +    typelib.writefd(fd)
> > +
> > +if __name__ == '__main__':
> > +    from optparse import OptionParser
> 
> How about you stick this all in a "def main()" and just call main() here, in
> case we get an opportunity to ditch pythonpath.py and still want to use this
> as a native command?

Ok.

> @@ +285,5 @@
> > +        print >>sys.stderr, "-d requires -o"
> > +        sys.exit(1)
> > +
> > +    if options.outfile is not None:
> > +        outfd = open(options.outfile, 'wb')
> 
> Is there a reason you open the file here and not in write_typelib? You'd
> only have to pass down the filename if you did that.

To match header.py.

> @@ +286,5 @@
> > +        sys.exit(1)
> > +
> > +    if options.outfile is not None:
> > +        outfd = open(options.outfile, 'wb')
> > +        closeoutfd = True
> 
> Feels like you just want to "with open(outfile, 'wb') as outfd". (You'll
> need from __future__ import with_statement to use that reliably.)

Again, just copied from header.py.  I think we should probably merge them eventually.

> @@ +299,5 @@
> > +    if closeoutfd:
> > +        outfd.close()
> > +
> > +    if options.depfile is not None:
> > +        depfd = open(options.depfile, 'w')
> 
> with open(options.depfile, 'w') as depfd: (Also it's a little weird to name
> these variables "fd", since they're really file objects. You can get real
> file descriptors in Python with os.open.)
> 
> ::: xpcom/typelib/xpt/tools/xpt.py
> @@ +145,5 @@
> >          self.pointer = pointer
> >          self.unique_pointer = unique_pointer
> >          self.reference = reference
> > +        if reference and not pointer:
> > +            raise Exception("If reference is True pointer must be True too")
> 
> Good catch.
> 
> @@ +645,5 @@
> >          self.optargc = optargc
> >          self.implicit_jscontext = implicit_jscontext
> >          self.params = list(params)
> > +        if result and not isinstance(result, Param):
> > +            raise Exception("result must be a Param!")
> 
> Good catch here too. Want to add unit tests for these two cases to the test
> suite?

I'll file a followup.

Comment 22

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

http://hg.mozilla.org/projects/build-system/rev/1a9b3ace8c68

Comment 23

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

http://hg.mozilla.org/mozilla-central/rev/1a9b3ace8c68

Comment 24

[Cellier fabien [:fcellier]]

I don't know if it's the good place but I had 2 small problems/regressions  when I switch from the old generator (source : https://hg.mozilla.org/projects/webcl/)

First,You can't chaining typedef anymore:
typedef long                    T_WebCLEnum;
typedef T_WebCLEnum             T_WebCLAddressingMode; // -> raise Unknow type


and conversions are different (python inside) :
  const long MY_TEST = 0xFFFFFFFF; doesn't work anymore
-> must write const long MY_TEST = -1;

I write that here in case someone has same issues.

Comment 25

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

Please file bugs with test case IDL files and assign them to me.

Comment 26

[Eric Shepherd [:sheppy]]

It would be helpful if someone could put together some basic notes (at least) on how this is used. Right now, the old xpidl is documented here: https://developer.mozilla.org/en/XPIDL/xpidl