Bug 182901 – Add SVGPropertyTraits::fromString() to all the SVG animated types

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Said Abou-Hallawa]

This is a code refactoring, It is part of the patch of https://bugs.webkit.org/show_bug.cgi?id=168586. The final goal is to templatize the SVGAnimatedType class and to use SVGPropertyTraits methods inside the template functions.

[Said Abou-Hallawa]

Created attachment 334101 [details]
Patch

[Said Abou-Hallawa]

Created attachment 334102 [details]
Patch

[Dean Jackson]

Comment on attachment 334102 [details]
Patch

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

> Source/WebCore/svg/SVGPathByteStream.h:60
> +    SVGPathByteStream(const SVGPathByteStream& other)
> +        : m_data(other.m_data)
>      {
> -        return m_data == other.m_data;
>      }
>  
> -    bool operator!=(const SVGPathByteStream& other) const
> +    SVGPathByteStream(SVGPathByteStream&& other)
> +        : m_data(WTFMove(other.m_data))
>      {
> -        return !(*this == other);
>      }

I'm not sure if we tend to write these in the style:

SVGPBS(SVGPBS&& other)
{
  *this = WTFMove(other);
}

Since we have operator= below.

> Source/WebCore/svg/properties/SVGPropertyTraits.h:52
> +        return color.isValid() ? color : std::optional<Color>();

Why isn't this 

return color.isValid() ? color : std::nullopt; ?

Or, if it can't be a nullopt, why does it return an optional?

Same for the other parse() functions below.

[Said Abou-Hallawa]

Created attachment 334203 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 334102 [details]
Patch

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

>> Source/WebCore/svg/SVGPathByteStream.h:60
>>      }
> 
> I'm not sure if we tend to write these in the style:
> 
> SVGPBS(SVGPBS&& other)
> {
>   *this = WTFMove(other);
> }
> 
> Since we have operator= below.

I do not think there is a style guidelines here but I changed the copy and the move constructors to use the copy and the move assignment operators.

>> Source/WebCore/svg/properties/SVGPropertyTraits.h:52
>> +        return color.isValid() ? color : std::optional<Color>();
> 
> Why isn't this 
> 
> return color.isValid() ? color : std::nullopt; ?
> 
> Or, if it can't be a nullopt, why does it return an optional?
> 
> Same for the other parse() functions below.

It has to return an optional because parse() may fail. The ternary operator does not like std::nullopt because its type isn't compatible with Color. The only way to use std::nullopt is to break the return statement into an if-else statement or

if (!color.isValid())
    return std::nullopt;
return color;

[WebKit Commit Bot]

Comment on attachment 334203 [details]
Patch

Clearing flags on attachment: 334203

Committed r228721: <https://trac.webkit.org/changeset/228721>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37693498>