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.
Created attachment 334101 [details] Patch
Created attachment 334102 [details] Patch
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.
Created attachment 334203 [details] Patch
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;
Comment on attachment 334203 [details] Patch Clearing flags on attachment: 334203 Committed r228721: <https://trac.webkit.org/changeset/228721>
All reviewed patches have been landed. Closing bug.
<rdar://problem/37693498>