842201 - SVGUnitTypes interface object is not being instantiated

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[David Zbarsky (:dzbarsky)]

I pushed SVGUnitTypes.webidl without [NoInterfaceObject], so just build and look at obj-dir/dom/bindings/SVGUnitTypes.cpp - it's empty.

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: webidl patch

Or just apply this patch if you don't want to pull

Comment 3

[Boris Zbarsky [:bzbarsky]]

This is sorta-kinda "fallout" from http://hg.mozilla.org/mozilla-central/rev/86cc9c289960 in that we still require a Bindings.conf entry for consequential stuff just like we usedto before that patch.

I think we should stop doing that.  Certainly at least for cases when hasInterfaceObject().  Want to do that?

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: Codegen changes

Comment 5

[David Zbarsky (:dzbarsky)]

Attachment: Remove nsIDOMSVGUnitTypes

Comment 6

[David Zbarsky (:dzbarsky)]

Attachment: Rmove nsIDOMSVGUnitTypes

Needed one more codegen change to avoid including nsIDOMSVGUnitTypes.h which no longer exists.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 721441 [details] [diff] [review]
Codegen changes

>+'ImplementedInterface' : {
>+        'headerFile': 'TestBindingHeader.h',
>+        'concrete': False,

Hmm.  I wonder whether we should default consequential interfaces to not concrete.  Maybe a followup bug?  Seems like having those be concrete would be the rare case.

>+            hasinstance = HASINSTANCE_HOOK_NAME if self.descriptor.interface.hasInterfacePrototypeObject() else "nullptr"

Er... no.  This looks wrong.  We should just be using HASINSTANCE_HOOK_NAME and then making it deal with the "not hasInterfacePrototypeObject()" case, shouldn't we?

Does "foo instanceof SVGUnitTypes" work correctly with your patches as they are?

>+            if d.interface.isExternal() or d.concrete or d.interface.getExtendedAttribute("PrefControlled") or not all(m.isConst() for m in d.interface.members):

This needs a nice comment.  And possibly this boolean should live on the descriptor itself so it can have a clear name.

>+        if not NeedsGeneratedHasInstance(self.descriptor) or not self.descriptor.interface.hasInterfacePrototypeObject():

Again, this looks wrong  What you want is to adjust the _code_ to not consider the proto object in the case when there isn't one, not disable the method altogether.

>+                if iface.isConsequential() and not iface.hasInterfaceObject():

Please fix the comment above this to reflect reality.

Given the [NoInterfaceObject] in TestCodeGen why do you need to add these interfaces to the conf file?  I guess it can't hurt....

r- because of the instanceof issue, I think.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 721455 [details] [diff] [review]
Rmove nsIDOMSVGUnitTypes

You can't do this without breaking instanceof (though you can remove all the members from the interface).

Comment 9

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 722041 [details] [diff] [review]
Patch

> +        implementationIncludes = set(d.headerFile for d in descriptors if d.needsHeaderFile())

Maybe call that method on Descriptor "needsHeaderInclude"?  It's about whether to try including the header, not whether the header is needed.

>+        """ if self.descriptor.interface.hasInterfacePrototypeObject() else ""

That's really easy to miss.  Please just do:

if self.descriptor.interface.hasInterfacePrototypeObject():
  hasInstanceCode = .....
else:
  hasInstanceCode = ""

>+  JSObject* instance = &vp.toObject();""" + hasInstanceCode + """

I'd prefer just having a %s in there that the hasInstanceCode gets substituted into.  Easier to read.

>+        return (self.interface.isExternal() or self.concrete or
>+        self.interface.getExtendedAttribute("PrefControlled") or
>+        not all(m.isConst() for m in self.interface.members))

Please fix up the indentation?

Add some tests?

r=me

Comment 11

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba0ef31a1726

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ba0ef31a1726