RESOLVED FIXED Bug 32910
Reverse JS GenerateConstructor logic 
https://bugs.webkit.org/show_bug.cgi?id=32910
Summary Reverse JS GenerateConstructor logic
Nikolas Zimmermann
Reported 2009-12-23 14:05:28 PST
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.
Attachments
Initial patch (38.97 KB, patch)
2009-12-23 15:07 PST, Nikolas Zimmermann 		no flags
DetailsFormatted DiffDiff
List of changed constructors (4.58 KB, text/plain)
2009-12-23 16:46 PST, Nikolas Zimmermann 		no flags
Details
Updated patch (44.14 KB, patch)
2009-12-23 16:46 PST, Nikolas Zimmermann 		eric: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Nikolas Zimmermann
Comment 1 2009-12-23 15:07:44 PST
Created attachment 45452 [details] Initial patch Patch implementing the changes described in the summary. No layout test regressions.
WebKit Review Bot
Comment 2 2009-12-23 15:09:49 PST
style-queue ran check-webkit-style on attachment 45452 [details] without any errors.
Eric Seidel (no email)
Comment 3 2009-12-23 15:39:42 PST
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
Comment 4 2009-12-23 15:45:05 PST
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)
Comment 5 2009-12-23 16:22:32 PST
(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
Comment 6 2009-12-23 16:28:38 PST
(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
Comment 7 2009-12-23 16:45:09 PST
(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
Comment 8 2009-12-23 16:46:02 PST
Created attachment 45454 [details] List of changed constructors New list of constructor changes.
Nikolas Zimmermann
Comment 9 2009-12-23 16:46:28 PST
Created attachment 45455 [details] Updated patch
Eric Seidel (no email)
Comment 10 2009-12-23 16:51:10 PST
Comment on attachment 45455 [details] Updated patch Looks OK. Thanks for taking this on!
WebKit Review Bot
Comment 11 2009-12-23 16:51:15 PST
style-queue ran check-webkit-style on attachment 45455 [details] without any errors.
Nikolas Zimmermann
Comment 12 2009-12-23 17:37:34 PST
Landed in r52534.
Dimitri Glazkov (Google)
Comment 13 2009-12-30 15:12:03 PST
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
Comment 14 2010-01-02 06:59:02 PST
(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 :-)
Note You need to log in before you can comment on or make changes to this bug.