Add support for IDL "implements" to Paris bindings

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Avast!  A pure-parser patch, with no codegen changes!
Created attachment 630301 [details] [diff] [review]
Add support for 'implements' in WebIDL.
Attachment #630301 - Flags: review?(khuey)
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.
> 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.
One other note.  I assume we should add webidl parser tests for this?  Just not sure what they would test...
I guess they could test some of the name collision stuff.
Created attachment 631030 [details] [diff] [review]
Add support for 'implements' in WebIDL.
Attachment #631030 - Flags: review?(khuey)
Attachment #630301 - Attachment is obsolete: true
Attachment #630301 - Flags: review?(khuey)
Blocks: 762654
Attachment #631030 - Flags: review?(khuey) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c2a635f4a26
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/8c2a635f4a26
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.