[webkit-reviews] review granted: [Bug 200565] XMLHttpRequest: getAllResponseHeaders() sorting : [Attachment 394172] 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 200565: XMLHttpRequest: getAllResponseHeaders() sorting
https://bugs.webkit.org/show_bug.cgi?id=200565

Attachment 394172: Patch

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




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

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

Neat that you did the case conversion manipulator! Nice job, too.

> Source/WTF/wtf/text/StringConcatenate.h:290
> +struct ToASCIICaseConvertor {

Seems like this name should be ASCIICaseConverter or
ASCIICaseConversionSpecification. I don’t know why we’d use the unconventional
spelling "convertor" instead of the more conventional "converter" and I don’t
think the converter converts "to" ASCII case, it converts ASCII case.

> Source/WTF/wtf/text/StringConcatenate.h:299
> +template<StringView::CaseConvertType type>
> +ToASCIICaseConvertor toASCIICase(StringView string)
> +{
> +    return { type, string };
> +}

I think it would be more elegant to have manipulators named "lowercase" and
"uppercase" or "lowercased" and "uppercased" rather than
toASCIICase<StringView::CaseConvertType::Lower> and
toASCIICase<StringView::CaseConvertType::Upper>. The analogy here is with
"pad".

> Source/WTF/wtf/text/StringConcatenate.h:313
> +	   WTF_STRINGTYPEADAPTER_COPIED_WTF_STRING();

This is not correct and should be omitted.

> Source/WTF/wtf/text/StringConcatenate.h:317
> +    const ToASCIICaseConvertor& m_toASCIICaseConvertor;

Just m_convertor or m_converter would be a fine name. There’s plenty of context
and we don’t need to repeat it all.

> Source/WTF/wtf/text/StringView.cpp:249
> +    static_assert(std::is_same<SourceCharacterType, LChar>::value ||
std::is_same<SourceCharacterType, UChar>::value);
> +    static_assert(std::is_same<DestinationCharacterType, LChar>::value ||
std::is_same<DestinationCharacterType, UChar>::value);

Could also static_assert that sizeof(DestinationCharacterType) <=
sizeof(SourceCharacterType), since this would not work properly if destination
was LChar and source was UChar.

> Source/WTF/wtf/text/StringView.cpp:251
> +	   destination[i] = type == StringView::CaseConvertType::Lower ?
toASCIILower(source[i]) : toASCIIUpper(source[i]);

Maybe this conditional should go outside the loop? I am not sure the compiler
will be smart enough to hoist the branch out of the loop.

> Source/WTF/wtf/text/StringView.h:123
> +    enum class CaseConvertType { Upper, Lower };

It’s quite inelegant that we have this separately in every string class! Should
clean that up at some point.

> Source/WebCore/xml/XMLHttpRequest.cpp:818
> +	      
headers.uncheckedAppend(std::make_pair(header.key.convertToASCIIUppercase(),
header.value));

No need to convert to ASCII uppercase. We can write a version of
WTF::codePointCompareLessThan that does that as part of the comparison process.
I mentioned that in my last review. Did you decide against it, or just miss the
comment?

> Source/WebCore/xml/XMLHttpRequest.cpp:821
> +	       return WTF::codePointCompareLessThan(x.first, y.first);

Here’s one cut at it:

    unsigned xLength = x.first.length();
    unsigned yLength = y.first.length();
    unsigned commonLength = std::min(xLength, yLength);
    for (unsigned i = 0; i < commonLength; ++i) {
	auto xCharacter = toASCIIUpper(x.first[i]);
	auto yCharacter = toASCIIUpper(y.first[i]);
	if (xCharacter != yCharacter)
	    return xCharacter < yCharacter);
    }
    return xLength < yLength;

Could optimize more, but maybe we don’t need to. The big win is not copying the
strings.