Bug 182901

Summary: Add SVGPropertyTraits::fromString() to all the SVG animated types
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: SVGAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, simon.fraser, thorton, webkit-bug-importer, zimmermann
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 168586    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Said Abou-Hallawa
Reported 2018-02-16 20:59:21 PST
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.
Attachments
Patch (70.68 KB, patch)
2018-02-16 21:31 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (70.41 KB, patch)
2018-02-16 21:48 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (70.65 KB, patch)
2018-02-19 16:48 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Said Abou-Hallawa
Comment 1 2018-02-16 21:31:38 PST
Created attachment 334101 [details] Patch
Said Abou-Hallawa
Comment 2 2018-02-16 21:48:50 PST
Created attachment 334102 [details] Patch
Dean Jackson
Comment 3 2018-02-19 13:41:03 PST
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
Comment 4 2018-02-19 16:48:01 PST
Created attachment 334203 [details] Patch
Said Abou-Hallawa
Comment 5 2018-02-19 16:55:45 PST
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 6 2018-02-19 18:03:02 PST
Comment on attachment 334203 [details] Patch Clearing flags on attachment: 334203 Committed r228721: <https://trac.webkit.org/changeset/228721>
WebKit Commit Bot
Comment 7 2018-02-19 18:03:04 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 8 2018-02-19 18:04:37 PST
<rdar://problem/37693498>
Note You need to log in before you can comment on or make changes to this bug.