[webkit-reviews] review granted: [Bug 180526] Update MIME type parser : [Attachment 362245] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Rob Buis <rbuis at igalia.com>'s request
for review:
Bug 180526: Update MIME type parser
https://bugs.webkit.org/show_bug.cgi?id=180526

Attachment 362245: Patch

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




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

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

Ideally this class needs to use StringView more and String less.

> Source/WebCore/platform/network/ParsedContentType.cpp:59
> +static Optional<SubstringRange> consumeCodePoints(const String& input,
unsigned& startIndex, CharacterMeetsCondition characterMeetsCondition, Mode
mode, bool skipTrailingWhitespace = false)

It’s inaccurate to name a function that processes code units, and does not
handle surrogate pairs where two code units make a single code point, with the
words "code points" in the function name. Also seems like a strangely
mechanical name. Is there some more human way to name it? Also, when passed
Mode::Rfc2045 the code explicitly calls isTokenCharacter, so the old name that
includes the word "token" is honest at least in that case. Based on all of that
I think I would leave this function name alone for now.

> Source/WebCore/platform/network/ParsedContentType.cpp:98
> +	   builder.append(input.substring(positionStart, position -
positionStart));

StringBuilder has a function for appending substrings that is more efficient
than first calling String::substring. To use it, write this:

    builder.append(input, positionStart, position - positionStart);

By writing it that way, we avoid allocating and destroying an extra StringImpl
every time, which is a poor pattern. Probably even better to just use a
StringView, in which case creating and destroying a StringImpl unnecessarily
would be a much more difficult mistake to make.

> Source/WebCore/platform/network/ParsedContentType.cpp:120
> +static String substringForRange(const String& string, const SubstringRange&
range)
> +{
> +    return string.substring(range.first, range.second);
> +}

This function, already existing, but being moved here, is an inefficient idiom.
It creates a temporary string, allocating a new StringImpl and copying the
string every time. For parsing, we should use StringView instead, which lets us
express substrings without allocating a new StringImpl each time as long as the
underlying String is not destroyed. It’s wasteful to use this function and
allocate a StringImpl, copy characters into it, use it to check if something is
a valid token, and then destroy the StringImpl. None of that would happen if we
used StringView instead.