761772 - Add support for IDL "implements" to Paris bindings

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Avast!  A pure-parser patch, with no codegen changes!

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Add support for 'implements' in WebIDL.

Comment 2

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

Comment on attachment 630301 [details] [diff] [review]
Add support for 'implements' in WebIDL.

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

::: dom/bindings/Makefile.in
@@ +119,5 @@
>    GlobalGen.py \
>    Bindings.conf \
>    Configuration.py \
>    Codegen.py \
> +  parser/WebIDL.py \

Shouldn't this be in bindinggen_dependencies too?

::: dom/bindings/parser/WebIDL.py
@@ +548,5 @@
>              self._extendedAttrDict[identifier] = attrlist if len(attrlist) else True
>  
> +    def addImplementedInterface(self, implementedInterface):
> +        assert(isinstance(implementedInterface, IDLInterface))
> +        self.implementedInterfaces.append(implementedInterface)

Please also assert that implementedInterface is not already in implementedInterfaces.

@@ +550,5 @@
> +    def addImplementedInterface(self, implementedInterface):
> +        assert(isinstance(implementedInterface, IDLInterface))
> +        self.implementedInterfaces.append(implementedInterface)
> +
> +    def getInheritedInterfaces(self):

Please document that this returns the inherited interfaces in order.

@@ +572,5 @@
> +        # And their inherited interfaces
> +        for iface in self.implementedInterfaces:
> +            implementedInterfaces.extend(iface.getInheritedInterfaces())
> +
> +        return implementedInterfaces

Don't we need to ensure that the members of implementedInterfaces are unique?

@@ +2155,5 @@
>          """
> +        assert(p[2] == "implements")
> +        implementor = IDLInterfacePlaceholder(self.getLocation(p, 1), p[1])
> +        implementee = IDLInterfacePlaceholder(self.getLocation(p, 3), p[3])
> +        p[0] = IDLImplementsStatement(self.getLocation(p, 1), implementor,

Why do we not even try to get the actual IDLInterface object?

@@ +3021,5 @@
> +        otherStatements = [ p for p in self._productions if
> +                            not isinstance(p, IDLImplementsStatement)]
> +        for production in implementsStatements:
> +            production.finish(self.globalScope())
> +        for production in otherStatements:

Please add an XXX comment here about how I'm going to remove this.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Shouldn't this be in bindinggen_dependencies too?

Yes.  Fixed.

> Please also assert that implementedInterface is not already in implementedInterfaces.

Done.

> Please document that this returns the inherited interfaces in order.

Done.

> Don't we need to ensure that the members of implementedInterfaces are unique?

I don't think we do, no.  If they're not, this will just fail the name-collision checks on members, I would think....

> Why do we not even try to get the actual IDLInterface object?

Because it would just mean we need more branches in the IDLImplementsStatement code?  What would be the benefit of trying to get the IDLInterface here?

> Please add an XXX comment here about how I'm going to remove this.

Done.

Comment 4

[Boris Zbarsky [:bzbarsky]]

One other note.  I assume we should add webidl parser tests for this?  Just not sure what they would test...

Comment 5

[Boris Zbarsky [:bzbarsky]]

I guess they could test some of the name collision stuff.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Add support for 'implements' in WebIDL.

Comment 7

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c2a635f4a26

Comment 8

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/8c2a635f4a26