[webkit-reviews] review granted: [Bug 224968] Extend SortedArrayMap further to work on case-folded strings, use in MIMETypeRegistry : [Attachment 427221] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Darin Adler <darin at apple.com>'s request
for review:
Bug 224968: Extend SortedArrayMap further to work on case-folded strings, use
in MIMETypeRegistry
https://bugs.webkit.org/show_bug.cgi?id=224968

Attachment 427221: Patch

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




--- Comment #5 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 427221
  --> https://bugs.webkit.org/attachment.cgi?id=427221
Patch

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

> Source/WTF/wtf/SortedArrayMap.h:117
> +// FIXME: Use std::all_of instead of this and remove it, once we require
C++20.
> +template<typename Iterator, typename Predicate> constexpr bool
allOfConstExpr(Iterator first, Iterator last, Predicate predicate)
> +{
> +    for (; first != last; ++first) {
> +	   if (!predicate(*first))
> +	       return false;
> +    }
> +    return true;
> +}

I think putting this in StdLibExtras.h would make more sense. And it gives us
fewer places to search when we upgrade to c++20. (There are probably things in
there we can remove now actually).

> Source/WTF/wtf/SortedArrayMap.h:202
> +template<typename CharacterType> inline bool lessThanASCIICaseFolding(const
CharacterType* characters, unsigned length, const char* literalWithNoUppercase)

Can this be constexpr?

> Source/WTF/wtf/SortedArrayMap.h:218
> +    if (string.is8Bit())
> +	   return lessThanASCIICaseFolding(string.characters8(),
string.length(), literalWithNoUppercase);
> +    return lessThanASCIICaseFolding(string.characters16(), string.length(),
literalWithNoUppercase);

Should we consider a utility on StringView/String that does this for us?

return string.withCharacters([&] (const auto* characters, unsigned length) {
    lessThanASCIICaseFolding(characters, length, literalWithNoUppercase);
});

It's the same number of lines, so I am not sure.

> Source/WebCore/platform/MIMETypeRegistry.cpp:78
> +constexpr ComparableCaseFoldingASCIILiteral supportedImageMIMETypeArray[] =
{

With a definition like this, where, e.g. what section, do these values end up
getting stored in the final binary? 

My feel for what constexpr on a global, with and without static, means is still
not super strong.

> Source/WebCore/platform/MIMETypeRegistry.cpp:243
> +constexpr ComparableLettersLiteral pdfMIMETypeArray[] = {
> +    "application/pdf",
> +    "text/pdf",
> +};
> +
> +FixedVector<const char*> MIMETypeRegistry::pdfMIMETypes()

Could this return a SortedArraySet<> to avoid any additional allocation?

> Tools/TestWebKitAPI/Tests/WTF/SortedArrayMap.cpp:97
> +    EXPECT_FALSE(caseFoldingSet.contains(""));
> +    EXPECT_TRUE(caseFoldingSet.contains("_"));
> +    EXPECT_TRUE(caseFoldingSet.contains("c"));
> +    EXPECT_TRUE(caseFoldingSet.contains("delightful"));
> +    EXPECT_FALSE(caseFoldingSet.contains("d"));
> +    EXPECT_TRUE(caseFoldingSet.contains("q_"));
> +    EXPECT_FALSE(caseFoldingSet.contains("q__"));
> +
> +    EXPECT_FALSE(lettersSet.contains(""));
> +    EXPECT_FALSE(lettersSet.contains("_"));
> +    EXPECT_TRUE(lettersSet.contains("c"));
> +    EXPECT_TRUE(lettersSet.contains("delightful"));
> +    EXPECT_FALSE(lettersSet.contains("d"));
> +    EXPECT_FALSE(lettersSet.contains("q_"));
> +    EXPECT_FALSE(lettersSet.contains("q__"));
> +
> +    ASSERT_TRUE(scriptTypesSet.contains("text/javascript"));
> +    ASSERT_TRUE(scriptTypesSet.contains("TEXT/JAVASCRIPT"));
> +    ASSERT_TRUE(scriptTypesSet.contains("application/javascript"));
> +    ASSERT_TRUE(scriptTypesSet.contains("application/ecmascript"));
> +    ASSERT_TRUE(scriptTypesSet.contains("application/x-javascript"));
> +    ASSERT_TRUE(scriptTypesSet.contains("application/x-ecmascript"));
> +    ASSERT_FALSE(scriptTypesSet.contains("text/plain"));
> +    ASSERT_FALSE(scriptTypesSet.contains("application/json"));
> +    ASSERT_FALSE(scriptTypesSet.contains("foo/javascript"));

I think having static_assert() variants would be good. Or maybe just switch
these all to static_asserts.