[webkit-reviews] review granted: [Bug 182901] Add SVGPropertyTraits::fromString() to all the SVG animated types : [Attachment 334102] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Feb 19 13:41:03 PST 2018


Dean Jackson <dino at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 182901: Add SVGPropertyTraits::fromString() to all the SVG animated types
https://bugs.webkit.org/show_bug.cgi?id=182901

Attachment 334102: Patch

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




--- Comment #3 from Dean Jackson <dino at apple.com> ---
Comment on attachment 334102
  --> https://bugs.webkit.org/attachment.cgi?id=334102
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.


More information about the webkit-reviews mailing list