Created attachment 344120 [details]
Patch

Comment on attachment 344120 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        This work is towards removing the SVG attributes macros.

It is great that you explained your plan. We tend to prefer to patches that both add a new data structure and include at least one actual code change that makes use of the new data structure. The latter provides a concrete example of how the data structure is used and reduces the likelihood that we are leave dead code in the repository in the unlikely event that you are unable to follow through with your plan.

> Source/WebCore/svg/properties/SVGAttribute.h:64
> +struct SVGPropertyAttribute : public SVGAttribute {
> +    SVGPropertyAttribute()
> +        : m_property(SVGPropertyTraits<PropertyType>::initialValue())
> +    {
> +    }
> +
> +    template<typename... Arguments>
> +    SVGPropertyAttribute(Arguments&&... arguments)
> +        : m_property(std::forward<Arguments>(arguments)...)
> +    {
> +    }
> +
> +    PropertyType& value() { return m_property; }
> +    const PropertyType& value() const { return m_property; }
> +
> +    void setValue(PropertyType&& property) { m_property = WTFMove(property); }
> +    void setValue(const PropertyType& property) { m_property = property; }
> +    void resetValue() { m_property = SVGPropertyTraits<PropertyType>::initialValue(); }
> +
> +    String toString() const { return SVGPropertyTraits<PropertyType>::toString(m_property); }
> +
> +    void setShouldSynchronize(bool shouldSynchronize) { m_shouldSynchronize = shouldSynchronize; }
> +    bool shouldSynchronize() const { return m_shouldSynchronize; }
> +    void synchronize(Element& element, const QualifiedName& attributeName)
> +    {
> +        if (!m_shouldSynchronize)
> +            return;
> +        element.setSynchronizedLazyAttribute(attributeName, toString());
> +    }

This is weird. We are underutilizing the purpose of the keyword "struct" and using it solely to avoid typing "public:\n". Having said that this data structure feels like a struct because it does not provide any encapsulation: it exposes all its instance data via setters and getters.

> Source/WebCore/svg/properties/SVGAttribute.h:79
> +    SVGAnimatedAttribute()
> +    {
> +    }

= default?

> Source/WebCore/svg/properties/SVGAttribute.h:96
> +    SVGAnimatedAttributeList()
> +    {
> +    }

= default?

Comment on attachment 344120 [details]
Patch

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

> Source/WebCore/svg/properties/SVGAttribute.h:33
> +struct SVGAttribute { };

"attribute" has a well-known meaning in the context of HTML/XML, and I wonder if using the term here is going to cause confusion.

The plan is submit all the SVG macro removal change as one patch although reviewing it will be done through multiple incremental patches in 186751.

*** This bug has been marked as a duplicate of bug 186751 ***