Bug 32910 – Reverse JS GenerateConstructor logic

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Always generate constructors for objects defined in an idl file, add new 'OmitConstructor' flag, allowing to restore the old behaviour.
The plan is to leave the GenerateConstructor statements in for now, and remove them in a follow-up patch, to reduce visual noise for this patch.

The patch I'm uploading soon, will just reverse the logic, resulting in no functional changes. The only exception is the svg/ directory, there are numerous of files which are supposed to generate constructors, which don't do this at the moment. Don't add "OmitConstructor" flags for those classes and don't expose them in DOMWindow so far. A follow-up patch should then expose these new constructors.

[Nikolas Zimmermann]

Created attachment 45452 [details]
Initial patch

Patch implementing the changes described in the summary. No layout test regressions.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45452 [details] without any errors.

[Eric Seidel (no email)]

OK.  In order to review this I need a list of classes to which you're adding constructors by this patch.  I would think that a little grep + sort + diff action could solve that.

[Nikolas Zimmermann]

Beautiful idea, to keep it as log here :-)
find . -type f -name "*.idl" | xargs grep GenerateConstructor | sed -e s/:.*// > WithGenCtor
find . -type f -name "*.idl" | xargs grep -L OmitConstructor  > WithoutOmitCtor
--- WithGenCtor	2009-12-24 00:43:14.000000000 +0100
+++ WithoutOmitCtor	2009-12-24 00:43:22.000000000 +0100
@@ -130,6 +130,7 @@
 ./html/HTMLOListElement.idl
 ./html/HTMLOptGroupElement.idl
 ./html/HTMLOptionElement.idl
+./html/HTMLOptionsCollection.idl
 ./html/HTMLParagraphElement.idl
 ./html/HTMLParamElement.idl
 ./html/HTMLPreElement.idl
@@ -161,29 +162,153 @@
 ./plugins/PluginArray.idl
 ./storage/Storage.idl
 ./storage/StorageEvent.idl
+./svg/ElementTimeControl.idl
+./svg/SVGAElement.idl
+./svg/SVGAltGlyphElement.idl
 ./svg/SVGAngle.idl
+./svg/SVGAnimateColorElement.idl
+./svg/SVGAnimatedAngle.idl
+./svg/SVGAnimatedBoolean.idl
+./svg/SVGAnimatedEnumeration.idl
+./svg/SVGAnimatedInteger.idl
+./svg/SVGAnimatedLength.idl
+./svg/SVGAnimatedLengthList.idl
+./svg/SVGAnimatedNumber.idl
+./svg/SVGAnimatedNumberList.idl
+./svg/SVGAnimatedPathData.idl
+./svg/SVGAnimatedPoints.idl
+./svg/SVGAnimatedPreserveAspectRatio.idl
+./svg/SVGAnimatedRect.idl
+./svg/SVGAnimatedString.idl
+./svg/SVGAnimatedTransformList.idl
+./svg/SVGAnimateElement.idl
+./svg/SVGAnimateTransformElement.idl
+./svg/SVGAnimationElement.idl
+./svg/SVGCircleElement.idl
+./svg/SVGClipPathElement.idl
 ./svg/SVGColor.idl
 ./svg/SVGComponentTransferFunctionElement.idl
+./svg/SVGCursorElement.idl
+./svg/SVGDefsElement.idl
+./svg/SVGDescElement.idl
+./svg/SVGDocument.idl
+./svg/SVGElement.idl
+./svg/SVGElementInstance.idl
+./svg/SVGElementInstanceList.idl
+./svg/SVGEllipseElement.idl
 ./svg/SVGException.idl
+./svg/SVGExternalResourcesRequired.idl
 ./svg/SVGFEBlendElement.idl
 ./svg/SVGFEColorMatrixElement.idl
+./svg/SVGFEComponentTransferElement.idl
 ./svg/SVGFECompositeElement.idl
+./svg/SVGFEDiffuseLightingElement.idl
 ./svg/SVGFEDisplacementMapElement.idl
+./svg/SVGFEDistantLightElement.idl
 ./svg/SVGFEFloodElement.idl
+./svg/SVGFEFuncAElement.idl
+./svg/SVGFEFuncBElement.idl
+./svg/SVGFEFuncGElement.idl
+./svg/SVGFEFuncRElement.idl
+./svg/SVGFEGaussianBlurElement.idl
+./svg/SVGFEImageElement.idl
+./svg/SVGFEMergeElement.idl
+./svg/SVGFEMergeNodeElement.idl
 ./svg/SVGFEMorphologyElement.idl
+./svg/SVGFEOffsetElement.idl
+./svg/SVGFEPointLightElement.idl
+./svg/SVGFESpecularLightingElement.idl
+./svg/SVGFESpotLightElement.idl
+./svg/SVGFETileElement.idl
 ./svg/SVGFETurbulenceElement.idl
+./svg/SVGFilterElement.idl
+./svg/SVGFilterPrimitiveStandardAttributes.idl
+./svg/SVGFitToViewBox.idl
+./svg/SVGFontElement.idl
+./svg/SVGFontFaceElement.idl
+./svg/SVGFontFaceFormatElement.idl
+./svg/SVGFontFaceNameElement.idl
+./svg/SVGFontFaceSrcElement.idl
+./svg/SVGFontFaceUriElement.idl
+./svg/SVGForeignObjectElement.idl
+./svg/SVGGElement.idl
+./svg/SVGGlyphElement.idl
 ./svg/SVGGradientElement.idl
+./svg/SVGHKernElement.idl
+./svg/SVGImageElement.idl
+./svg/SVGLangSpace.idl
 ./svg/SVGLength.idl
+./svg/SVGLengthList.idl
+./svg/SVGLinearGradientElement.idl
+./svg/SVGLineElement.idl
+./svg/SVGLocatable.idl
 ./svg/SVGMarkerElement.idl
+./svg/SVGMaskElement.idl
+./svg/SVGMatrix.idl
+./svg/SVGMetadataElement.idl
+./svg/SVGMissingGlyphElement.idl
+./svg/SVGNumber.idl
+./svg/SVGNumberList.idl
 ./svg/SVGPaint.idl
+./svg/SVGPathElement.idl
 ./svg/SVGPathSeg.idl
+./svg/SVGPathSegArcAbs.idl
+./svg/SVGPathSegArcRel.idl
+./svg/SVGPathSegClosePath.idl
+./svg/SVGPathSegCurvetoCubicAbs.idl
+./svg/SVGPathSegCurvetoCubicRel.idl
+./svg/SVGPathSegCurvetoCubicSmoothAbs.idl
+./svg/SVGPathSegCurvetoCubicSmoothRel.idl
+./svg/SVGPathSegCurvetoQuadraticAbs.idl
+./svg/SVGPathSegCurvetoQuadraticRel.idl
+./svg/SVGPathSegCurvetoQuadraticSmoothAbs.idl
+./svg/SVGPathSegCurvetoQuadraticSmoothRel.idl
+./svg/SVGPathSegLinetoAbs.idl
+./svg/SVGPathSegLinetoHorizontalAbs.idl
+./svg/SVGPathSegLinetoHorizontalRel.idl
+./svg/SVGPathSegLinetoRel.idl
+./svg/SVGPathSegLinetoVerticalAbs.idl
+./svg/SVGPathSegLinetoVerticalRel.idl
+./svg/SVGPathSegList.idl
+./svg/SVGPathSegMovetoAbs.idl
+./svg/SVGPathSegMovetoRel.idl
+./svg/SVGPatternElement.idl
+./svg/SVGPoint.idl
+./svg/SVGPointList.idl
+./svg/SVGPolygonElement.idl
+./svg/SVGPolylineElement.idl
 ./svg/SVGPreserveAspectRatio.idl
+./svg/SVGRadialGradientElement.idl
+./svg/SVGRect.idl
+./svg/SVGRectElement.idl
 ./svg/SVGRenderingIntent.idl
+./svg/SVGScriptElement.idl
+./svg/SVGSetElement.idl
+./svg/SVGStopElement.idl
+./svg/SVGStringList.idl
+./svg/SVGStylable.idl
+./svg/SVGStyleElement.idl
+./svg/SVGSVGElement.idl
+./svg/SVGSwitchElement.idl
+./svg/SVGSymbolElement.idl
+./svg/SVGTests.idl
 ./svg/SVGTextContentElement.idl
+./svg/SVGTextElement.idl
 ./svg/SVGTextPathElement.idl
+./svg/SVGTextPositioningElement.idl
+./svg/SVGTitleElement.idl
 ./svg/SVGTransform.idl
+./svg/SVGTransformable.idl
+./svg/SVGTransformList.idl
+./svg/SVGTRefElement.idl
+./svg/SVGTSpanElement.idl
 ./svg/SVGUnitTypes.idl
+./svg/SVGURIReference.idl
+./svg/SVGUseElement.idl
+./svg/SVGViewElement.idl
+./svg/SVGViewSpec.idl
 ./svg/SVGZoomAndPan.idl
+./svg/SVGZoomEvent.idl
 ./workers/AbstractWorker.idl
 ./workers/WorkerLocation.idl
 ./xml/DOMParser.idl
@@ -195,12 +320,3 @@
 ./xml/XPathException.idl
 ./xml/XPathExpression.idl
 ./xml/XPathResult.idl
-./xml/DOMParser.idl
-./xml/XMLHttpRequestException.idl
-./xml/XMLHttpRequestProgressEvent.idl
-./xml/XMLHttpRequestUpload.idl
-./xml/XMLSerializer.idl
-./xml/XPathEvaluator.idl
-./xml/XPathException.idl
-./xml/XPathExpression.idl
-./xml/XPathResult.idl

[Eric Seidel (no email)]

(In reply to comment #4)
> Beautiful idea, to keep it as log here :-)
We could probably do so as attachments instead, given the size. :)

These all seem abstract, should they really have constructors?
> +./svg/ElementTimeControl.idl
> +./svg/SVGAnimationElement.idl
> +./svg/SVGExternalResourcesRequired.idl
> +./svg/SVGFitToViewBox.idl
> +./svg/SVGLangSpace.idl
> +./svg/SVGLocatable.idl
> +./svg/SVGStylable.idl
> +./svg/SVGTests.idl
> +./svg/SVGTransformable.idl
> +./svg/SVGURIReference.idl

I'm also confused by these.  Can you explain?
> -./xml/DOMParser.idl
> -./xml/XMLHttpRequestException.idl
> -./xml/XMLHttpRequestProgressEvent.idl
> -./xml/XMLHttpRequestUpload.idl
> -./xml/XMLSerializer.idl
> -./xml/XPathEvaluator.idl
> -./xml/XPathException.idl
> -./xml/XPathExpression.idl
> -./xml/XPathResult.idl

[Nikolas Zimmermann]

(In reply to comment #5)
> (In reply to comment #4)
> > Beautiful idea, to keep it as log here :-)
> We could probably do so as attachments instead, given the size. :)
> 
> These all seem abstract, should they really have constructors?
> > +./svg/ElementTimeControl.idl
> > +./svg/SVGAnimationElement.idl
> > +./svg/SVGExternalResourcesRequired.idl
> > +./svg/SVGFitToViewBox.idl
> > +./svg/SVGLangSpace.idl
> > +./svg/SVGLocatable.idl
> > +./svg/SVGStylable.idl
> > +./svg/SVGTests.idl
> > +./svg/SVGTransformable.idl
> > +./svg/SVGURIReference.idl

Ok, I would have changed that in a follow-up patch, after the one removing the remaining GenerateConstructor flags, but it's probably better to land the right changes from the beginning. Will fix.

> 
> I'm also confused by these.  Can you explain?
> > -./xml/DOMParser.idl
> > -./xml/XMLHttpRequestException.idl
> > -./xml/XMLHttpRequestProgressEvent.idl
> > -./xml/XMLHttpRequestUpload.idl
> > -./xml/XMLSerializer.idl
> > -./xml/XPathEvaluator.idl
> > -./xml/XPathException.idl
> > -./xml/XPathExpression.idl
> > -./xml/XPathResult.idl

These files did not contain GenerateConstructor, and contain OmitConstructor now, so they show up as minus lines when asking for a diff between "files with GenerateConstructor set" and "files without OmitConstructor set". I think it's just fine?

[Nikolas Zimmermann]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Beautiful idea, to keep it as log here :-)
> > We could probably do so as attachments instead, given the size. :)
> > 
> > These all seem abstract, should they really have constructors?
> > > +./svg/ElementTimeControl.idl
> > > +./svg/SVGAnimationElement.idl
> > > +./svg/SVGExternalResourcesRequired.idl
> > > +./svg/SVGFitToViewBox.idl
> > > +./svg/SVGLangSpace.idl
> > > +./svg/SVGLocatable.idl
> > > +./svg/SVGStylable.idl
> > > +./svg/SVGTests.idl
> > > +./svg/SVGTransformable.idl
> > > +./svg/SVGURIReference.idl
> 
> Ok, I would have changed that in a follow-up patch, after the one removing the
> remaining GenerateConstructor flags, but it's probably better to land the right
> changes from the beginning. Will fix.
> 
> > 
> > I'm also confused by these.  Can you explain?
> > > -./xml/DOMParser.idl
> > > -./xml/XMLHttpRequestException.idl
> > > -./xml/XMLHttpRequestProgressEvent.idl
> > > -./xml/XMLHttpRequestUpload.idl
> > > -./xml/XMLSerializer.idl
> > > -./xml/XPathEvaluator.idl
> > > -./xml/XPathException.idl
> > > -./xml/XPathExpression.idl
> > > -./xml/XPathResult.idl
> 
> These files did not contain GenerateConstructor, and contain OmitConstructor
> now, so they show up as minus lines when asking for a diff between "files with
> GenerateConstructor set" and "files without OmitConstructor set". I think it's
> just fine?

Oops, it was a bug in the generator script of these changes, sorry for the confusion, Please ignore the - lines, they are junk.

[Nikolas Zimmermann]

Created attachment 45454 [details]
List of changed constructors

New list of constructor changes.

[Nikolas Zimmermann]

Created attachment 45455 [details]
Updated patch

[Eric Seidel (no email)]

Comment on attachment 45455 [details]
Updated patch

Looks OK.  Thanks for taking this on!

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45455 [details] without any errors.

[Nikolas Zimmermann]

Landed in r52534.

[Dimitri Glazkov (Google)]

In some of these cases, OmitConstructor isn't quite true: there is a constructor, but it's not simple (like WebKitCSSMatrix or WebGLArrayBuffer).

What do you think about using WebIDL's notion of the Constructor attribute (http://www.w3.org/TR/WebIDL/#Constructor) here? I know it's a reverse of what you've done so far, but it seems more accurate.

[Nikolas Zimmermann]

(In reply to comment #13)
> In some of these cases, OmitConstructor isn't quite true: there is a
> constructor, but it's not simple (like WebKitCSSMatrix or WebGLArrayBuffer).
We could introduce a special marker "UseCustomConstructor" to make it more clear for those classes. But I don't think we should add another marker, just for those two cases.

> 
> What do you think about using WebIDL's notion of the Constructor attribute
> (http://www.w3.org/TR/WebIDL/#Constructor) here? I know it's a reverse of what
> you've done so far, but it seems more accurate.

We agreed before to always add constructors, except for cases where you don't want a constructor (OmitConstructor) or wheter you're using a custom constructor.

I followed Sam Weinig proposal here, if anyone wants to change it. No problem with me, just needs discussion :-)