[webkit-reviews] review granted: [Bug 213635] Convert SVG related parsers over to using StringParsingBuffer : [Attachment 402906] 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 213635: Convert SVG related parsers over to using StringParsingBuffer
https://bugs.webkit.org/show_bug.cgi?id=213635

Attachment 402906: Patch

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




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

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

> Source/WebCore/svg/SVGAnimationElement.cpp:81
> +static Optional<Vector<UnitBezier>> parseKeySplines(const StringView&
string)

Shouldn't we prefer StringView over const StringView&?

> Source/WebCore/svg/SVGAnimationElement.cpp:84
> +    if (string.isEmpty())
>	   return WTF::nullopt;

Is this a valuable optimization? I don’t think it is. Or is it needed for
correctness.

> Source/WebCore/svg/SVGFitToViewBox.cpp:88
>  Optional<FloatRect> SVGFitToViewBox::parseViewBox(const StringView& value)

Shouldn't we prefer StringView over const StringView&?

> Source/WebCore/svg/SVGLengthList.h:55
> +    bool parse(const String& value);

Any downside in changing this to take StringView?

> Source/WebCore/svg/SVGLengthValue.cpp:71
> +    auto firstChar = *buffer;
> +    ++buffer;

Why not *buffer++?

> Source/WebCore/svg/SVGLengthValue.cpp:77
> +    auto secondChar = *buffer;
> +    ++buffer;

Ditto.

> Source/WebCore/svg/SVGLengthValue.h:66
> +    static Optional<SVGLengthValue> construct(SVGLengthMode, const
StringView&);
> +    static SVGLengthValue construct(SVGLengthMode, const StringView&,
SVGParsingError&, SVGLengthNegativeValuesMode =
SVGLengthNegativeValuesMode::Allow);

StringView instead of const StringView&?

> Source/WebCore/svg/SVGLengthValue.h:90
> +    ExceptionOr<void> setValueAsString(const StringView&);
> +    ExceptionOr<void> setValueAsString(const StringView&, SVGLengthMode);

StringView instead of const StringView&?

> Source/WebCore/svg/SVGNumberList.cpp:65
> +    StringBuilder builder;
> +
> +    for (const auto& number : m_items) {
> +	   if (builder.length())
> +	       builder.append(' ');
> +
> +	   builder.append(number->value());
> +    }
> +
> +    return builder.toString();

Another idiom we should find a way to improve. I don’t like how we have to
write these join loops.

> Source/WebCore/svg/SVGNumberList.h:53
> +    bool parse(const String&);

StringView?

> Source/WebCore/svg/SVGNumberList.h:54
> +    String valueAsString() const override;

final?

> Source/WebCore/svg/SVGParserUtilities.cpp:117
> +	       exponent += *buffer - '0';
> +	       ++buffer;

*buffer++?

> Source/WebCore/svg/SVGPathSource.h:22
> +#include "FloatPoint.h"

Makes me a tiny bit sad. Not sure what made this happen.

> Source/WebCore/svg/SVGPathStringSource.h:49
> +    template<typename F> decltype(auto) parse(F&&);

I’d prefer a word rather than "F". Is this a function?

> Source/WebCore/svg/SVGPointList.h:53
> +    bool parse(const String& value);

StringView?

> Source/WebCore/svg/SVGPreserveAspectRatioValue.cpp:365
> +	   case SVG_PRESERVEASPECTRATIO_NONE:
> +	       return "none";
> +	   case SVG_PRESERVEASPECTRATIO_XMINYMIN:
> +	       return "xMinYMin";
> +	   case SVG_PRESERVEASPECTRATIO_XMIDYMIN:
> +	       return "xMidYMin";
> +	   case SVG_PRESERVEASPECTRATIO_XMAXYMIN:
> +	       return "xMaxYMin";
> +	   case SVG_PRESERVEASPECTRATIO_XMINYMID:
> +	       return "xMinYMid";
> +	   case SVG_PRESERVEASPECTRATIO_XMIDYMID:
> +	       return "xMidYMid";
> +	   case SVG_PRESERVEASPECTRATIO_XMAXYMID:
> +	       return "xMaxYMid";
> +	   case SVG_PRESERVEASPECTRATIO_XMINYMAX:
> +	       return "xMinYMax";
> +	   case SVG_PRESERVEASPECTRATIO_XMIDYMAX:
> +	       return "xMidYMax";
> +	   case SVG_PRESERVEASPECTRATIO_XMAXYMAX:
> +	       return "xMaxYMax";
> +	   case SVG_PRESERVEASPECTRATIO_UNKNOWN:
> +	       return "unknown";

Want some "_s" here I think. Unless the ASCIILiteral optimization is
unimportant.

> Source/WebCore/svg/SVGPreserveAspectRatioValue.h:55
> +    SVGPreserveAspectRatioValue(const StringView&);

StringView rather than const StringView&?

> Source/WebCore/svg/SVGPreserveAspectRatioValue.h:67
> +    bool parse(const StringView&);

StringView rather than const StringView&?

> Source/WebCore/svg/SVGPreserveAspectRatioValue.h:78
> +    template<typename CharacterType>
> +    bool parseInternal(StringParsingBuffer<CharacterType>&, bool validate);

On one line?

> Source/WebCore/svg/SVGStringList.h:54
> +    bool parse(const String& data, UChar delimiter);

StringView

> Source/WebCore/svg/SVGTransformList.cpp:59
> +    for (const auto& transform : m_items)

I would have omitted the const here.

> Source/WebCore/svg/SVGTransformList.h:61
> +    void parse(const String&);

StringView

> Source/WebCore/svg/SVGTransformable.cpp:100
> +constexpr int requiredValuesForType[] =  {0, 6, 1, 1, 1, 1, 1};
> +constexpr int optionalValuesForType[] =  {0, 0, 1, 1, 2, 0, 0};

Remove the extra space before "{"?

Not sure we should have removed static. Because constexpr does imply const, but
not necessarily internal linkage. Unclear on the considerations.

> Source/WebCore/svg/SVGTransformable.h:37
> +    static Optional<SVGTransformValue::SVGTransformType>
parseTransformType(const StringView&);

StringView rather than const StringView&?