| Summary: | Reverse JS GenerateConstructor logic | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Nikolas Zimmermann <zimmermann> | ||||||||
| Component: | WebCore JavaScript | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | dglazkov, eric, sam, staikos, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Nikolas Zimmermann
2009-12-23 14:05:28 PST
Created attachment 45452 [details]
Initial patch
Patch implementing the changes described in the summary. No layout test regressions.
style-queue ran check-webkit-style on attachment 45452 [details] without any errors.
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. 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 (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 (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? (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. Created attachment 45454 [details]
List of changed constructors
New list of constructor changes.
Created attachment 45455 [details]
Updated patch
Comment on attachment 45455 [details]
Updated patch
Looks OK. Thanks for taking this on!
style-queue ran check-webkit-style on attachment 45455 [details] without any errors.
Landed in r52534. 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. (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 :-) |