Bug 32910 - Reverse JS GenerateConstructor logic
Summary: Reverse JS GenerateConstructor logic
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-23 14:05 PST by Nikolas Zimmermann
Modified: 2010-01-02 06:59 PST (History)
5 users (show)

See Also:


Attachments
Initial patch (38.97 KB, patch)
2009-12-23 15:07 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
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+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 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.
Comment 1 Nikolas Zimmermann 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.
Comment 2 WebKit Review Bot 2009-12-23 15:09:49 PST
style-queue ran check-webkit-style on attachment 45452 [details] without any errors.
Comment 3 Eric Seidel (no email) 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.
Comment 4 Nikolas Zimmermann 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
Comment 5 Eric Seidel (no email) 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
Comment 6 Nikolas Zimmermann 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?
Comment 7 Nikolas Zimmermann 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.
Comment 8 Nikolas Zimmermann 2009-12-23 16:46:02 PST
Created attachment 45454 [details]
List of changed constructors

New list of constructor changes.
Comment 9 Nikolas Zimmermann 2009-12-23 16:46:28 PST
Created attachment 45455 [details]
Updated patch
Comment 10 Eric Seidel (no email) 2009-12-23 16:51:10 PST
Comment on attachment 45455 [details]
Updated patch

Looks OK.  Thanks for taking this on!
Comment 11 WebKit Review Bot 2009-12-23 16:51:15 PST
style-queue ran check-webkit-style on attachment 45455 [details] without any errors.
Comment 12 Nikolas Zimmermann 2009-12-23 17:37:34 PST
Landed in r52534.
Comment 13 Dimitri Glazkov (Google) 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.
Comment 14 Nikolas Zimmermann 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 :-)