[webkit-reviews] review granted: [Bug 186751] Remove the SVG elements' attributes macros : [Attachment 346124] Introduce SVGAttributeRegistry

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 186751: Remove the SVG elements' attributes macros
https://bugs.webkit.org/show_bug.cgi?id=186751

Attachment 346124: Introduce SVGAttributeRegistry

https://bugs.webkit.org/attachment.cgi?id=346124&action=review




--- Comment #66 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 346124
  --> https://bugs.webkit.org/attachment.cgi?id=346124
Introduce SVGAttributeRegistry

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

> Source/WebCore/ChangeLog:9
> +	   attributes of a given owner. It takes into account the inheritance
of the

By "owner" I think you mean a given attribute identified by a name/type pair?
It's not that every instance of an attribute has a unique registry entry, but
this is static data that is one per attribute class?

> Source/WebCore/svg/properties/SVGAttributeRegistry.h:48
> +	   auto it = std::find_if(m_map.begin(), m_map.end(),
[&attributeName](const auto& entry) -> bool {
> +	       return entry.key.matches(attributeName);
> +	   });
> +	   return it != m_map.end();

Why isn't this just m_map.contains()? Seems like you're doing manual hash key
iteration here?

> Source/WebCore/svg/properties/SVGAttributeRegistry.h:62
> +	   for (auto* attributeAccessor : m_map.values())
> +	       attributeAccessor->synchronizeProperty(owner, element);
> +	       synchronizeAttributesBaseTypes(owner, element);

The indentation is off here.

> Source/WebCore/svg/properties/SVGAttributeRegistry.h:84
> +    template<size_t I = 0>
> +    static typename std::enable_if<I == sizeof...(BaseTypes),
Vector<AnimatedPropertyType>>::type animatedTypesBaseTypes(const
QualifiedName&) { return { }; }
> +
> +    template<size_t I = 0>
> +    static typename std::enable_if<I < sizeof...(BaseTypes),
Vector<AnimatedPropertyType>>::type animatedTypesBaseTypes(const QualifiedName&
attributeName)

I think this template magic deserves some comments to explain how it works.