[webkit-reviews] review granted: [Bug 213416] Convert much of the SVG string parsing code to use Optional based return values rather than out-parameters : [Attachment 402407] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 213416: Convert much of the SVG string parsing code to use Optional based
return values rather than out-parameters
https://bugs.webkit.org/show_bug.cgi?id=213416

Attachment 402407: Patch

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




--- Comment #11 from Darin Adler <darin at apple.com> ---
Comment on attachment 402407
  --> https://bugs.webkit.org/attachment.cgi?id=402407
Patch

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

Another one of those patches where I have a local git branch with lots of
nearly identical changes. Very happy to see this done!

> Source/WebCore/svg/SVGAngleValue.cpp:109
> +    auto helper = [&](const auto* ptr, const auto* end) -> ExceptionOr<void>
{

I would probably just do auto twice here.

Noticed that whether there is a space after "]" and before "(" I snot the same
here as in another recent patch.

> Source/WebCore/svg/SVGAngleValue.cpp:125
> +	   const auto* ptr = value.characters8();
> +	   const auto* end = ptr + value.length();

Just auto maybe instead of const auto*?

Also, no need for "end" local variable.

> Source/WebCore/svg/SVGAngleValue.cpp:130
> +    const auto* ptr = value.characters16();
> +    const auto* end = ptr + value.length();

Ditto.

>> Source/WebCore/svg/SVGAnimateMotionElement.cpp:165
>>	m_hasToPointAtEndOfDuration = true;
> 
> Looks like m_toPointAtEndOfDuration could probably be an optional as well?

"true"

> Source/WebCore/svg/SVGAnimationElement.cpp:183
> -	   parseKeySplines(value, m_keySplines);
> +	   if (auto keySplines = parseKeySplines(value))
> +	       m_keySplines = WTFMove(*keySplines);
> +	   else
> +	       m_keySplines.clear();

Does this preserve the old behavior?

Also, seems like some refactoring could make this a one-liner instead of an if
statement. Maybe valueOr or making more things Optional?

>> Source/WebCore/svg/SVGFEImageElement.cpp:-121
>> -	    preserveAspectRatio.parse(value);
> 
> What happened to this parse call?

The parsing is now part of the constructor that takes a string.

> Source/WebCore/svg/SVGFitToViewBox.cpp:74
> +	   if (!value.isNull()) {
> +	       if (auto result = parseViewBox(value)) {
> +		   setViewBox(WTFMove(*result));
> +		   return true;
> +	       }
> +	   }

Does the null check help? Maybe parseViewBox will relatively quickly fail when
passed a null string and we don’t need this optimization.

> Source/WebCore/svg/SVGFitToViewBox.h:69
> +    Optional<FloatRect> parseViewBox(const AtomString& value);
> +    Optional<FloatRect> parseViewBox(const UChar*& start, const UChar* end,
bool validate = true);

Peculiar to have a parse function take an AtomString. I suggest changing it to
take a StringView. Then we would not need any overloading; callers of both of
the current functions could call a new single one.

> Source/WebCore/svg/SVGHKernElement.cpp:49
> +    // FIXME: Can this be shared with
SVGVKernElement::buildVerticalKerningPair

Missing question mark as the end of this question.

> Source/WebCore/svg/SVGHKernElement.cpp:84
> +    SVGKerningPair kerningPair;
> +    kerningPair.glyphName1 = WTFMove(*glyphName1);
> +    kerningPair.glyphName2 = WTFMove(*glyphName2);
> +    kerningPair.unicodeRange1 = WTFMove(unicodeString1->first);
> +    kerningPair.unicodeName1 = WTFMove(unicodeString1->second);
> +    kerningPair.unicodeRange2 = WTFMove(unicodeString2->first);
> +    kerningPair.unicodeName2 = WTFMove(unicodeString2->second);
> +    kerningPair.kerning = kerning;
> +    return kerningPair;

Is construction rather than assignment a possibility here?

> Source/WebCore/svg/SVGParserUtilities.h:57
> +Optional<float> parseNumber(const LChar*& current, const LChar* end,
SuffixSkippingPolicy = SuffixSkippingPolicy::Skip);
> +Optional<float> parseNumber(const UChar*& current, const UChar* end,
SuffixSkippingPolicy = SuffixSkippingPolicy::Skip);
> +Optional<float> parseNumber(const String&, SuffixSkippingPolicy =
SuffixSkippingPolicy::Skip);
> +
> +Optional<std::pair<float, float>> parseNumberOptionalNumber(const String&);
> +
> +Optional<bool> parseArcFlag(const LChar*& current, const LChar* end);
> +Optional<bool> parseArcFlag(const UChar*& current, const UChar* end);
> +
> +Optional<FloatPoint> parsePoint(const String&);
> +Optional<FloatRect> parseRect(const String&);
> +
> +Optional<FloatPoint> parseFloatPoint(const LChar*& current, const LChar*
end);
> +Optional<FloatPoint> parseFloatPoint(const UChar*& current, const UChar*
end);
> +
> +Optional<std::pair<UnicodeRanges, HashSet<String>>>
parseKerningUnicodeString(const String&);
> +Optional<HashSet<String>> parseGlyphName(const String&);

Seems like these functions should all take StringView. Except I guess the ones
that "advance a pointer" can’t yet. Certainly the ones that take String should
likely take a StringView unless String creates some optimization opportunity.

> Source/WebCore/svg/SVGParserUtilities.h:62
> +template<typename CharacterType> inline bool isSVGSpace(CharacterType c)

constexpr instead of inline

> Source/WebCore/svg/SVGParserUtilities.h:67
> +template<typename CharacterType> inline bool skipOptionalSVGSpaces(const
CharacterType*& ptr, const CharacterType* end)

Not sure we need to say inline. It’s not like more inlining will occur with the
keyword than without it.

Also, this function seems closely related to other skipUntil functions
elsewhere.

> Source/WebCore/svg/SVGParserUtilities.h:74
> +template<typename CharacterType> inline bool
skipOptionalSVGSpacesOrDelimiter(const CharacterType*& ptr, const
CharacterType* end, char delimiter = ',')

Ditto.

> Source/WebCore/svg/SVGPathBlender.cpp:131
> +    auto parsedTo = std::invoke(function, toSource);

I think we should WTFMove the function here. Can’t safely do it above, but can
do it here.

> Source/WebCore/svg/SVGVKernElement.cpp:73
> +    bool ok = false;
> +    auto kerning =
attributeWithoutSynchronization(SVGNames::kAttr).string().toFloat(&ok);
> +    if (!ok)
> +	   return WTF::nullopt;

I think we have an optional-based float parsing API somewhere instead of the
"out argument OK boolean one". If not, future refactoring I guess.

> Source/WebCore/svg/SVGVKernElement.cpp:83
> +    SVGKerningPair kerningPair;
> +    kerningPair.glyphName1 = WTFMove(*glyphName1);
> +    kerningPair.glyphName2 = WTFMove(*glyphName2);
> +    kerningPair.unicodeRange1 = WTFMove(unicodeString1->first);
> +    kerningPair.unicodeName1 = WTFMove(unicodeString1->second);
> +    kerningPair.unicodeRange2 = WTFMove(unicodeString2->first);
> +    kerningPair.unicodeName2 = WTFMove(unicodeString2->second);
> +    kerningPair.kerning = kerning;
> +    return kerningPair;

Same question about constructing rather than assigning.

> Source/WebCore/svg/properties/SVGAnimationAdditiveValueFunctionImpl.h:258
> +	   float fromNumber = parseNumber(from).valueOr(0);
> +	   float toNumber = parseNumber(to).valueOr(0);

Not sure we need local variables. One liner would be nicer.

> Source/WebCore/svg/properties/SVGAnimationAdditiveValueFunctionImpl.h:259
>	   return fabsf(toNumber - fromNumber);

std::abs better than fabsf.

> Source/WebCore/svg/properties/SVGPropertyTraits.h:81
> +	   return std::make_pair(static_cast<int>(roundf(result->first)),
static_cast<int>(roundf(result->second)));

std::round better than roundf