Closed Bug 761772 Opened 9 years ago Closed 9 years ago

Add support for IDL "implements" to Paris bindings

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Avast!  A pure-parser patch, with no codegen changes!
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.
Attachment #630301 - Attachment is obsolete: true
Attachment #630301 - Flags: review?(khuey)
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
Closed: 9 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.