Bug 63797 – Add a possibility to retrieve the associated SVGAnimatedProperty object for a certain XML attribute

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Add a possibility to retrieve the associated SVGAnimatedProperty object for a certain XML attribute.
Take SVGRectElement as example. I want a method that returns "widthAnimated()" when given the SVGNames::widthAttr. That's needed for our animVal support.

Unfortunately that would mean having to hand-write a lot of code, that looks similar to the existing fillAttributeToPropertyTypeMap/synchronizeProperty.
While looking for a better approach I found a possibility to remove all occurrences of synchronizeProperty/fillAttributeToPropertyTypeMap in the SVG*Element subclasses.

[Nikolas Zimmermann]

Created attachment 99828 [details]
Patch

Jeez, 500k pure code changes. Yes, I'm taking the brown paper bag.

[WebKit Review Bot]

Attachment 99828 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Last 3072 characters of output:
whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:69:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:70:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:70:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:71:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:72:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:73:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:74:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:76:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:77:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:78:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:79:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:79:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:80:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:81:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:82:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:83:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:84:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:85:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:86:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:87:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:88:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:89:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/properties/SVGAnimatedProperty.h:94:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/SVGUseElement.h:108:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/SVGUseElement.h:109:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/SVGUseElement.h:110:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/SVGUseElement.h:111:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/SVGUseElement.h:112:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 47 in 168 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 99828 [details]
Patch

Attachment 99828 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8986772

[WebKit Review Bot]

Comment on attachment 99828 [details]
Patch

Attachment 99828 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8986773

[Early Warning System Bot]

Comment on attachment 99828 [details]
Patch

Attachment 99828 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8986774

[Nikolas Zimmermann]

Created attachment 99831 [details]
Patch v2

Fix merging problems due recent SVGAnimateElement.cpp changes.

[WebKit Review Bot]

Attachment 99831 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/svg/SVGViewSpec.h:60:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/SVGFEDiffuseLightingElement.h:53:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/SVGFEDiffuseLightingElement.h:54:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/SVGFEDiffuseLightingElement.h:55:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/SVGFEDiffuseLightingElement.h:56:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/SVGTests.cpp:187:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/svg/properties/SVGAttributeToPropertyMap.cpp:23:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/WebCore/svg/SVGFELightElement.h:49:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/SVGFELightElement.h:50:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/SVGFELightElement.h:51:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/SVGFELightElement.h:52:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/SVGFELightElement.h:53:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/SVGFELightElement.h:54:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/SVGFELightElement.h:55:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/SVGFELightElement.h:56:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/SVGFELightElement.h:57:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 16 in 168 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Created attachment 99835 [details]
Patch v3

Fix last style issues.

[WebKit Review Bot]

Comment on attachment 99835 [details]
Patch v3

Attachment 99835 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8988760

[Rob Buis]

Comment on attachment 99835 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=99835&action=review

Just a first quick review, I think the ChangeLog can be improved a lot, especially the enum addition/changes.

> Source/WebCore/ChangeLog:46
> +        These lookups are all performed dynamically. Each synchronizeProperty() call does a lot of comparisions.

comparisions -> comparisons

> Source/WebCore/ChangeLog:58
> +        Using these information, we can replace all the various synchronizeProperty/fillAttributeToPropertyMap implementations, with a single one in SVGElement.

Using this information

> Source/WebCore/ChangeLog:102
> +            This is the main piece of the logic that replaces the manual synchronizeProperty/fillAttributeToPropertyMap implementation. It expans to following:

expans -> expands

> Source/WebCore/ChangeLog:137
> +                A generic way to synchronize a SVGAnimatedProperty with its XML DOM attribute. Any SVG DOM change to eg. <rects> x property will now trigger

<rects> -> <rect>s

> Source/WebCore/ChangeLog:147
> +                This method is not used yet, but allows us to collect all SVGAnimatedProperies for a QualifiedName -- the initial goal for this patch.

SVGAnimatedProperies -> SVGAnimatedProperties

> Source/WebCore/ChangeLog:562
> +        (WebCore::SVGPropertyTearOff::updateAnimVal):

These may need more in detail explanations. The one big thing I couldn't find in the ChangeLog is why the enums like SVG_TEXTPATH_SPACINGTYPE_AUTO <-> TextPathSpacingAuto change was done? You hint at it in the comments but I think it should be in the ChangeLog.

[Rob Buis]

Comment on attachment 99835 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=99835&action=review

Just a first quick review, I think the ChangeLog can be improved a lot, especially the enum addition/changes. r- because of my enum request and build failures mac-ews.

>> Source/WebCore/ChangeLog:46
>> +        These lookups are all performed dynamically. Each synchronizeProperty() call does a lot of comparisions.
> 
> comparisions -> comparisons

comparisions -> comparisons

>> Source/WebCore/ChangeLog:58
>> +        Using these information, we can replace all the various synchronizeProperty/fillAttributeToPropertyMap implementations, with a single one in SVGElement.
> 
> Using this information

Using this information

>> Source/WebCore/ChangeLog:102
>> +            This is the main piece of the logic that replaces the manual synchronizeProperty/fillAttributeToPropertyMap implementation. It expans to following:
> 
> expans -> expands

expans -> expands

>> Source/WebCore/ChangeLog:137
>> +                A generic way to synchronize a SVGAnimatedProperty with its XML DOM attribute. Any SVG DOM change to eg. <rects> x property will now trigger
> 
> <rects> -> <rect>s

<rects> -> <rect>s

>> Source/WebCore/ChangeLog:147
>> +                This method is not used yet, but allows us to collect all SVGAnimatedProperies for a QualifiedName -- the initial goal for this patch.
> 
> SVGAnimatedProperies -> SVGAnimatedProperties

SVGAnimatedProperies -> SVGAnimatedProperties

>> Source/WebCore/ChangeLog:562
>> +        (WebCore::SVGPropertyTearOff::updateAnimVal):
> 
> These may need more in detail explanations. The one big thing I couldn't find in the ChangeLog is why the enums like SVG_TEXTPATH_SPACINGTYPE_AUTO <-> TextPathSpacingAuto change was done? You hint at it in the comments but I think it should be in the ChangeLog.

These may need more in detail explanations. The one big thing I couldn't find in the ChangeLog is why the enums like SVG_TEXTPATH_SPACINGTYPE_AUTO <-> TextPathSpacingAuto change was done? You hint at it in the comments but I think it should be in the ChangeLog.

[Nikolas Zimmermann]

Thanks for the first review Rob, I'll split this up into more pieces!

[Nikolas Zimmermann]

(In reply to comment #10)
> (From update of attachment 99835 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99835&action=review
> 
> Just a first quick review, I think the ChangeLog can be improved a lot, especially the enum addition/changes.
Ok. I've splitted out the enum parts into a seperated patch, it already landed, thanks to Dirks review.
Fixed all typos you mentioned as well.

> 
> > Source/WebCore/ChangeLog:562
> > +        (WebCore::SVGPropertyTearOff::updateAnimVal):
> 
> These may need more in detail explanations. The one big thing I couldn't find in the ChangeLog is why the enums like SVG_TEXTPATH_SPACINGTYPE_AUTO <-> TextPathSpacingAuto change was done? You hint at it in the comments but I think it should be in the ChangeLog.

Oops, the whole updateAnimVal changes, the isAnimating methods etc all don't belong in this patch, I've removed all of that -- it belongs in the follow-up patch which actually implements animVal.

[Nikolas Zimmermann]

Created attachment 99986 [details]
Patch

[WebKit Review Bot]

Comment on attachment 99986 [details]
Patch

Attachment 99986 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8995431

[Nikolas Zimmermann]

Created attachment 99999 [details]
Patch v5

Fix release builds.

[Dirk Schulze]

Comment on attachment 99986 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99986&action=review

I have general concerns about our new naming schema (not only in your patch). Properties should be CSS properties and attributes every XML attribute IMO. Not sure if it is a good idea to name it all properties. But it is indeed difficult, since we have attributes that are properties as well and some that are just attributes.

I'd like Rob to look at the makros as well.

> Source/WebCore/svg/SVGAElement.h:72
> -    DECLARE_ANIMATED_BOOLEAN(ExternalResourcesRequired, externalResourcesRequired)
> +     BEGIN_DECLARE_ANIMATED_PROPERTIES(SVGAElement)
> +          // This declaration used to define a non-virtual "String& target() const" method, that clashes with "virtual String Element::target() const".
> +          // That's why it has been renamed to "svgTarget", the CodeGenerators take care of calling svgTargetAnimated() instead of targetAnimated(), see CodeGenerator.pm.
> +          DECLARE_ANIMATED_STRING(SVGTarget, svgTarget)
> +          DECLARE_ANIMATED_STRING(Href, href)
> +          DECLARE_ANIMATED_BOOLEAN(ExternalResourcesRequired, externalResourcesRequired)
> +     END_DECLARE_ANIMATED_PROPERTIES

Indentation wrong.

> Source/WebCore/svg/SVGAnimateTransformElement.cpp:79
> +    Vector<AnimatedPropertyType> propertyTypes;
> +    targetElement->animatedPropertyTypeForAttribute(attributeName(), propertyTypes);

Why is this a vector? Is AnimatedPropertyType a SVGTransform here?

> Source/WebCore/svg/SVGAnimateTransformElement.cpp:80
> +    if (propertyTypes.isEmpty() || propertyTypes[0] != AnimatedTransformList)

Why do we just care about the first item?

> Source/WebCore/svg/SVGElement.h:130
> +    virtual bool haveLoadedRequiredResources();    

extra spaces are superfluous

> Source/WebCore/svg/SVGMarkerElement.cpp:38
> +// Define custom animated property 'orientType'.
> +const SVGPropertyInfo* SVGMarkerElement::orientTypePropertyInfo()
> +{

Is this done because of value 'auto'? Can you comment about this problem? IIRC we had problems with 'auto' and the different types that are mapped to SVGAngle.

[Nikolas Zimmermann]

(In reply to comment #17)
> (From update of attachment 99986 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99986&action=review
> 
> I have general concerns about our new naming schema (not only in your patch). Properties should be CSS properties and attributes every XML attribute IMO.

In CSS everything is called a "property". In SVG there's a clear distinction vs.
"readonly SVGAnimatedLength x" which is a SVG property, and "<rect x=...>" where x is an attribute.
When I speak of SVG properties, I mean those reachable through SVG DOM, everything else is an attribute (xAttr, etc..)

> Not sure if it is a good idea to name it all properties. But it is indeed difficult, since we have attributes that are properties as well and some that are just attributes.
Aha, this I think is the source of confusion. We don't have attributes that are properties, etc. This is the wrong way to think about it.
We have CSS properties, SVG properties and XML DOM attributes.
Some SVG properties are _mapped_ to XML DOM attributes and vice-versa.
That's the idea behind the REGISTER_ANIMATED macros. We use it to map a SVG property to a XML DOM attribute or more than one SVG property to a single DOM attribute (eg. orientAttr).

> 
> I'd like Rob to look at the makros as well.
Sure.

> 
> > Source/WebCore/svg/SVGAElement.h:72
> > -    DECLARE_ANIMATED_BOOLEAN(ExternalResourcesRequired, externalResourcesRequired)
> > +     BEGIN_DECLARE_ANIMATED_PROPERTIES(SVGAElement)
> > +          // This declaration used to define a non-virtual "String& target() const" method, that clashes with "virtual String Element::target() const".
> > +          // That's why it has been renamed to "svgTarget", the CodeGenerators take care of calling svgTargetAnimated() instead of targetAnimated(), see CodeGenerator.pm.
> > +          DECLARE_ANIMATED_STRING(SVGTarget, svgTarget)
> > +          DECLARE_ANIMATED_STRING(Href, href)
> > +          DECLARE_ANIMATED_BOOLEAN(ExternalResourcesRequired, externalResourcesRequired)
> > +     END_DECLARE_ANIMATED_PROPERTIES
> 
> Indentation wrong.
Fixed.

> > Source/WebCore/svg/SVGAnimateTransformElement.cpp:79
> > +    Vector<AnimatedPropertyType> propertyTypes;
> > +    targetElement->animatedPropertyTypeForAttribute(attributeName(), propertyTypes);
> 
> Why is this a vector? Is AnimatedPropertyType a SVGTransform here?
Because a single DOM attribute may map to multiple SVG properties.
orientAttr (DOM Attribute) -> orientAngle/orientType (SVG properties)

> 
> > Source/WebCore/svg/SVGAnimateTransformElement.cpp:80
> > +    if (propertyTypes.isEmpty() || propertyTypes[0] != AnimatedTransformList)
> 
> Why do we just care about the first item?
Because there's a 1:1 mapping between transformAttr and the transform property.

> 
> > Source/WebCore/svg/SVGElement.h:130
> > +    virtual bool haveLoadedRequiredResources();    
> 
> extra spaces are superfluous
Fixed.

> 
> > Source/WebCore/svg/SVGMarkerElement.cpp:38
> > +// Define custom animated property 'orientType'.
> > +const SVGPropertyInfo* SVGMarkerElement::orientTypePropertyInfo()
> > +{
> 
> Is this done because of value 'auto'? Can you comment about this problem? IIRC we had problems with 'auto' and the different types that are mapped to SVGAngle.
It's an optimization to NOT use DECLARE_ANIMATED_ENUMERATION for orientType.

Say I'm using:
myMarker.orientAngle.baseVal = someAngleWith10deg;
myMarker.orientType.baseVal = MARKER_ORIENT_AUTO;

Then I'll do alert(myMarker.getAttribute('orient')) in order to synchronize the SVG DOM with the XML DOM. First a call to  "synchronizeOrientAngle" would be triggered and then a call to "synchronizeOrientType".

The first call to syncOrientAngle would set "orient" to "10deg". The next call to synchronizeOrientAngle would set "orient" to "auto". In general, this would work, but in order to save the extra work I'm implementing synchronizeOrientType this way:

void SVGMarkerElement::synchronizeOrientType(void* contextElement)
{
    ASSERT(contextElement);
    SVGMarkerElement* ownerType = static_cast<SVGMarkerElement*>(contextElement);
    if (!ownerType->m_orientType.shouldSynchronize)
        return;

    // If orient is not auto, the previous call to synchronizeOrientAngle already set the orientAttr to the right angle.
    if (ownerType->m_orientType.value != SVGMarkerOrientAuto)
        return;

    DEFINE_STATIC_LOCAL(AtomicString, autoString, ("auto"));
    SVGAnimatedPropertySynchronizer<true>::synchronize(ownerType, orientTypePropertyInfo()->attributeName, autoString);
}

Note, that this was slightly different in the patch I uploaded - but I just realized, I did it wrong, this is the right version which avoids the extra work (as synchronizeOrientAngle is guaranteed to be called before synchronizeOrientType). Uploading a new patch that addresses all your comments.

Good that you brought this up again!

[Nikolas Zimmermann]

Created attachment 100111 [details]
Patch v6

[Dirk Schulze]

Comment on attachment 100111 [details]
Patch v6

View in context: https://bugs.webkit.org/attachment.cgi?id=100111&action=review

Looks great in general, just some questions.

> Source/WebCore/ChangeLog:1182
>  2011-07-07  Nikolas Zimmermann  <[e-mail]>
>  
> +        Add a possibility to retrieve the associated SVGAnimatedProperty object for a certain XML attribute
> +        https://bugs.webkit.org/show_bug.cgi?id=63797
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        In order to prepare animVal support we need a way to map a given SVG DOM attribute to a SVGAnimatedProperty.
> +        eg. SVGNames::xAttr -> SVGRectElement::xAnimated(), etc. This will be needed to update the animVal of the
> +        SVGAnimatedProperty, if an animation is running. It would required adding a new method to all SVG* classes
> +        that define animated properties. Unfortunately we already have lots of repeated code in methods like
> +        synchronizeProperty / fillAttributeToPropertyTypeMap. Look at SVGRectElement for example:
> +
> +        void SVGRectElement::synchronizeProperty(const QualifiedName& attrName)
> +        {

remove the double post please.

> Source/WebCore/svg/SVGTests.cpp:49
> +}
> +
> +

Omit one new line here.

> Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:103
> +#define BEGIN_REGISTER_ANIMATED_PROPERTIES(OwnerType) \

You are creating the Map and the caller. BEGIN seems not to be the best choice. Together with the END_REGISTER makro it looks more like a class or namespace that surrounds the other content of the REGISTER_* makros. What about CREATE_ANIMATED_PROPERTIES_MAP or something similar?

> Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:123
> +#define END_REGISTER_ANIMATED_PROPERTIES }

Ah you are really surrounding code. Now I see. Hm. I still think that BEGIN_REGISTER_ANIMATED_PROPERTIES does not tell enough about what it is doing. Leave the END_REGISTER and rename the other makro.

> Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:126
> +#define BEGIN_DEFINE_ANIMATED_PROPERTIES

I may miss something, why do you define empty Makros? I guess they are just placeholders for the upcoming animation patches? If so please add a comment here. I'd still like to know how they should look like. Otherwise I don't know if the naming is correct.

> Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:132
> +        s_propertyInfo = new SVGPropertyInfo(AnimatedPropertyTypeEnum, \

do you free the static variable somewhere?

> Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:141
> +#define END_DEFINE_ANIMATED_PROPERTIES

See comment above.

> Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:144
> +#define BEGIN_DECLARE_ANIMATED_PROPERTIES(OwnerType) \

naming sounds ok here.

> Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:160
> +    \

Can you omit the white space please?

> Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:165
> +    \

Ditto.

> Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:170
> +    \

Ditto.

> Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:176
> +    \

continues. I'll stop here.

> Source/WebCore/svg/properties/SVGAttributeToPropertyMap.h:44
> +    void addProperties(SVGAttributeToPropertyMap& map)
> +    {
> +        AttributeToPropertiesMap::iterator end = map.m_map.end();
> +        for (AttributeToPropertiesMap::iterator it = map.m_map.begin(); it != end; ++it) {

I think this should be moved into a cpp.

[Nikolas Zimmermann]

(In reply to comment #20)
> (From update of attachment 100111 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100111&action=review
> 
> Looks great in general, just some questions.
Sure!

> 
> > Source/WebCore/ChangeLog:1182
> remove the double post please.
Fixed.

> 
> > Source/WebCore/svg/SVGTests.cpp:49
> Omit one new line here.
Fixed.

> > Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:103
> > +#define BEGIN_REGISTER_ANIMATED_PROPERTIES(OwnerType) \
> 
> You are creating the Map and the caller. BEGIN seems not to be the best choice. Together with the END_REGISTER makro it looks more like a class or namespace that surrounds the other content of the REGISTER_* makros. What about CREATE_ANIMATED_PROPERTIES_MAP or something similar?
> 
> > Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:123
> > +#define END_REGISTER_ANIMATED_PROPERTIES }
> 
> Ah you are really surrounding code. Now I see. Hm. I still think that BEGIN_REGISTER_ANIMATED_PROPERTIES does not tell enough about what it is doing. Leave the END_REGISTER and rename the other makro.

I chose BEGIN / END for consistency, I think it should stay this way.

> 
> > Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:126
> > +#define BEGIN_DEFINE_ANIMATED_PROPERTIES
> 
> I may miss something, why do you define empty Makros? I guess they are just placeholders for the upcoming animation patches? If so please add a comment here. I'd still like to know how they should look like. Otherwise I don't know if the naming is correct.

It's intentionally empty, I added it to have consistent wrapping around the various DECLARE/DEFINE/REGISTER_ANIMATED_PROPERTY macros.

> 
> > Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:132
> > +        s_propertyInfo = new SVGPropertyInfo(AnimatedPropertyTypeEnum, \
> 
> do you free the static variable somewhere?
Nope, this is the same as using DEFINE_STATIC_LOCAL. I'll switch to use DEFINE_STATIC_LOCAL to hide this detail.

> 
> > Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:141
> > +#define END_DEFINE_ANIMATED_PROPERTIES
> 
> See comment above.
As I said, I only added those to have a consistent style:
BEGIN_FOO
   FOO_PROPERTY
...
END_FOO

In my initial version of this patch, I didn't have these empty macros, but I really like it now! :-)


> > Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:144
> > +#define BEGIN_DECLARE_ANIMATED_PROPERTIES(OwnerType) \
> 
> naming sounds ok here.
> 
> > Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:160
> > +    \
> 
> Can you omit the white space please?
...
> > Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:176
...
> continues. I'll stop here.

Why? This makes the macros more readable, as the whole stuff is not so compact? At least I find it this way.

> 
> > Source/WebCore/svg/properties/SVGAttributeToPropertyMap.h:44
> > +    void addProperties(SVGAttributeToPropertyMap& map)
> > +    {
> > +        AttributeToPropertiesMap::iterator end = map.m_map.end();
> > +        for (AttributeToPropertiesMap::iterator it = map.m_map.begin(); it != end; ++it) {
> 
> I think this should be moved into a cpp.
Okay, will fix.

[Dirk Schulze]

(In reply to comment #21)
> (In reply to comment #20)
> > You are creating the Map and the caller. BEGIN seems not to be the best choice. Together with the END_REGISTER makro it looks more like a class or namespace that surrounds the other content of the REGISTER_* makros. What about CREATE_ANIMATED_PROPERTIES_MAP or something similar?
...
> > Ah you are really surrounding code. Now I see. Hm. I still think that BEGIN_REGISTER_ANIMATED_PROPERTIES does not tell enough about what it is doing. Leave the END_REGISTER and rename the other makro.
> 
> I chose BEGIN / END for consistency, I think it should stay this way.
Independent of consistency, Makros already hide a lot of code. So at  least the names should say what they are good for.

> 
> > 
> > > Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:126
> > > +#define BEGIN_DEFINE_ANIMATED_PROPERTIES
> > 
> > I may miss something, why do you define empty Makros? I guess they are just placeholders for the upcoming animation patches? If so please add a comment here. I'd still like to know how they should look like. Otherwise I don't know if the naming is correct.
> 
> It's intentionally empty, I added it to have consistent wrapping around the various DECLARE/DEFINE/REGISTER_ANIMATED_PROPERTY macros.
I think that is a bad idea. Every developer thinks about logic behind makros. It just doesn't make sense to add empty makros just because they look nice IMO.

> 
> > 
> > > Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:132
> > > +        s_propertyInfo = new SVGPropertyInfo(AnimatedPropertyTypeEnum, \
> > 
> > do you free the static variable somewhere?
> Nope, this is the same as using DEFINE_STATIC_LOCAL. I'll switch to use DEFINE_STATIC_LOCAL to hide this detail.
Ah ok, great.

> BEGIN_FOO
>    FOO_PROPERTY
> ...
> END_FOO
> 
> In my initial version of this patch, I didn't have these empty macros, but I really like it now! :-)
See comment above.

> 
> 
> > > Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:144
> > > +#define BEGIN_DECLARE_ANIMATED_PROPERTIES(OwnerType) \
> > 
> > naming sounds ok here.
> > 
> > > Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:160
> > > +    \
> > 
> > Can you omit the white space please?
> ...
> > > Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:176
> ...
> > continues. I'll stop here.
> 
> Why? This makes the macros more readable, as the whole stuff is not so compact? At least I find it this way.
We avoid that on every file. We have dozens of cleanup patches removing whitespaces. So why should we do it on generated code? Nevertheless, I don't care that much about it, just think it is not a clean style. You can leave it if you want.

[Nikolas Zimmermann]

Created attachment 100180 [details]
Patch v7

Removed the empty macros and the now unnecessary GetOwnerElementForType helper struct -- adapt CodeGenerators*.pm which were the last users of it.
This should fix all comments Dirk raised.

[Dirk Schulze]

Comment on attachment 100180 [details]
Patch v7

View in context: https://bugs.webkit.org/attachment.cgi?id=100180&action=review

Looks great. r=me. Just:

> Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:78
> +#define BEGIN_REGISTER_ANIMATED_PROPERTIES(OwnerType) \

think about if you don't want to rename it like I mentioned in a previous comment.

[Nikolas Zimmermann]

Landed in r90680. Thanks Dirk for the review!

[Nikolas Zimmermann]

Landed WinCE build fix in r90681.