[webkit-reviews] review granted: [Bug 215671] Implement Request/Response consuming as FormData : [Attachment 407003] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 215671: Implement Request/Response consuming as FormData
https://bugs.webkit.org/show_bug.cgi?id=215671

Attachment 407003: Patch

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




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

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

> Source/WTF/wtf/text/WTFString.cpp:862
> +    constexpr auto function = replaceInvalidSequences ?
convertUTF8ToUTF16ReplacingInvalidSequences : convertUTF8ToUTF16;

Not sure the constexpr here has any effect.

> Source/WTF/wtf/text/WTFString.cpp:863
> +    if (!function(stringCurrent, reinterpret_cast<const char *>(stringStart
+ length), &bufferCurrent, bufferCurrent + buffer.size(), nullptr))

Non-WebKit-code-style space was here in "char *".

> Source/WTF/wtf/unicode/UTF8Conversion.h:48
> +WTF_EXPORT_PRIVATE bool convertUTF8ToUTF16ReplacingInvalidSequences(const
char* sourceStart, const char* sourceEnd, UChar** targetStart, UChar*
targetEnd, bool* isSourceAllASCII);

What was behind the choice to not include the nullptr default for the last
argument?

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:48
> +static bool containsOnlyHTTPQuotedStringTokenCodePoints(const String&
string)

Seems like we’d want the argument to be StringView, not String.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:59
> +    for (size_t i = 0; i < string.length(); i++) {
> +	   if (!isHTTPQuotedStringTokenCodePoint(string[i]))
> +	       return false;
> +    }
> +    return true;

The isAllSpecialCharacters template (which could also be named containsOnly)
was designed for use in cases like this so we would not write a loop.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:68
> +static HashMap<String, String> parseParameters(const String& input, size_t
position)

Seems like we’d want the argument to be StringView, not String.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:109
> +static Optional<MimeType> parseMIMEType(const String& contentType)

Seems like we’d want the argument to be StringView, not String.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:140
> +#if PLATFORM(WIN)
> +const void* memmem(const void* haystack, size_t haystackLength, const void*
needle, size_t needleLength)
> +{
> +    const char* pointer = static_cast<const char*>(haystack);
> +    while (haystackLength >= needleLength) {
> +	   if (!memcmp(pointer, needle, needleLength))
> +	       return pointer;
> +	   pointer++;
> +	   haystackLength--;
> +    }
> +    return nullptr;
> +}
> +#endif

In the past we put functions from <string.h> that are missing on Windows in
<wtf/StringExtras.h>, and I suggest moving it there. If here, it should be
marked static. If there, inline.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:162
> +	   size_t contentDispsitionParametersBegin = header.find(';',
contentDispositionBegin + strlen(contentDispositionCharacters));

"disposition" is misspelled here.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:166
> +	   auto parameters =
parseParameters(header.substring(contentDispsitionParametersBegin,
contentDispositionEnd - contentDispsitionParametersBegin), 0);

Seems unnecessary to construct a WTF::String and allocate another copy in
memory just to parse a substring. This is what StringView is for.