Last Comment Bug 761772 - Add support for IDL "implements" to Paris bindings
: Add support for IDL "implements" to Paris bindings
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: ParisBindings 762654
  Show dependency treegraph
 
Reported: 2012-06-05 13:23 PDT by Boris Zbarsky [:bz]
Modified: 2012-06-12 03:04 PDT (History)
1 user (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add support for 'implements' in WebIDL. (17.22 KB, patch)
2012-06-05 13:24 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review
Add support for 'implements' in WebIDL. (23.15 KB, patch)
2012-06-07 10:23 PDT, Boris Zbarsky [:bz]
khuey: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] 2012-06-05 13:23:25 PDT
Avast!  A pure-parser patch, with no codegen changes!
Comment 1 Boris Zbarsky [:bz] 2012-06-05 13:24:39 PDT
Created attachment 630301 [details] [diff] [review]
Add support for 'implements' in WebIDL.
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-06 14:34:13 PDT
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 [:bz] 2012-06-06 14:42:52 PDT
> 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 [:bz] 2012-06-06 20:16:21 PDT
One other note.  I assume we should add webidl parser tests for this?  Just not sure what they would test...
Comment 5 Boris Zbarsky [:bz] 2012-06-06 20:16:45 PDT
I guess they could test some of the name collision stuff.
Comment 6 Boris Zbarsky [:bz] 2012-06-07 10:23:13 PDT
Created attachment 631030 [details] [diff] [review]
Add support for 'implements' in WebIDL.
Comment 8 Graeme McCutcheon [:graememcc] 2012-06-12 03:04:53 PDT
https://hg.mozilla.org/mozilla-central/rev/8c2a635f4a26

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