Closed Bug 842201 Opened 7 years ago Closed 7 years ago

SVGUnitTypes interface object is not being instantiated

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
I pushed SVGUnitTypes.webidl without [NoInterfaceObject], so just build and look at obj-dir/dom/bindings/SVGUnitTypes.cpp - it's empty.
Attached patch webidl patch (obsolete) — Splinter Review
Or just apply this patch if you don't want to pull
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?
Summary: Remove nsIDOMSVGUnitTypes → SVGUnitTypes interface object is not being instantiated
Attached patch Codegen changes (obsolete) — Splinter Review
Attachment #715001 - Attachment is obsolete: true
Attachment #721441 - Flags: review?(bzbarsky)
Attached patch Remove nsIDOMSVGUnitTypes (obsolete) — Splinter Review
Attachment #721442 - Flags: review?(bzbarsky)
Attached patch Rmove nsIDOMSVGUnitTypes (obsolete) — Splinter Review
Needed one more codegen change to avoid including nsIDOMSVGUnitTypes.h which no longer exists.
Attachment #721442 - Attachment is obsolete: true
Attachment #721442 - Flags: review?(bzbarsky)
Attachment #721455 - Flags: review?(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.
Attachment #721441 - Flags: review?(bzbarsky) → review+
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).
Attachment #721455 - Flags: review?(bzbarsky) → review-
Attached patch PatchSplinter Review
Attachment #721441 - Attachment is obsolete: true
Attachment #721455 - Attachment is obsolete: true
Attachment #722041 - Flags: review?(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
Attachment #722041 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/ba0ef31a1726
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.