[webkit-reviews] review granted: [Bug 193909] Implement serializing in MIME type parser : [Attachment 361430] 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 193909: Implement serializing in MIME type parser
https://bugs.webkit.org/show_bug.cgi?id=193909

Attachment 361430: Patch

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




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

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

Looks great. Could probably pile on a few more test cases.

> Source/WebCore/platform/network/ParsedContentType.cpp:332
> -	   m_mimeType.convertToASCIILowercase();
> +	   m_mimeType = m_mimeType.convertToASCIILowercase();

Wow, this seems like a major bug. The old code didn’t do anything at all. I
guess your new regression tests cover this.

> Source/WebCore/platform/network/ParsedContentType.cpp:351
> -	   keyName.convertToASCIILowercase();
> +	   name = name.convertToASCIILowercase();

Again!

> Source/WebCore/platform/network/ParsedContentType.cpp:361
> +    for (auto paramName : m_parameterNames) {

Using auto here causes unnecessary reference count churn. You could use auto&
instead and there would not be any.

Goes against WebKit coding style to call this "paramName". Writing out the
whole word "parameter" doesn’t make it that much longer. But also in the
context of this sort function, I think just "name" would be even better than
"parameterName" (or "paramName").

> Source/WebCore/platform/network/ParsedContentType.cpp:366
> +	   if (value.isEmpty() || value.find(isNonTokenCharacter) != notFound)
{

Unfortunate that we don’t have a "contains" overload, because find(x) !=
notFound seems a little harder to read.

> Tools/TestWebKitAPI/Tests/WebCore/ParsedContentType.cpp:105
> +    return String();

Might be nice to return something like "ERROR" here so it’s easier to see the
difference between a parse failure, empty string, and null string in test cases
below. Might want to do similar for null vs. empty string?

> Tools/TestWebKitAPI/Tests/WebCore/ParsedContentType.cpp:110
> +    EXPECT_EQ(serializeIfValid("text/plain"), "text/plain");

When tests are about strings, I find it better to use EXPECT_STREQ so that the
errors are easier to understand. That requires a strategy for extending the
lifetime of a const char*; some of our tests use global buffers since there is
no need for the tests to work multi-threaded, others call utf8().data(), others
even use std::string and c_str(). You can see examples of multiple approaches
in various test files such as TextCodec.cpp, HashMap.cpp and many others.

> Tools/TestWebKitAPI/Tests/WebCore/ParsedContentType.cpp:120
> +    EXPECT_EQ(serializeIfValid("/plain"), String());

I typically always include test cases like empty string and null string, even
if they don't make any sense, just to make sure the function doesn’t cash in
those cases. Similarly I’d suggest having test that is just ";".

> Tools/TestWebKitAPI/Tests/WebCore/ParsedContentType.cpp:123
> +    EXPECT_EQ(serializeIfValid("text/plain;test"), "text/plain");

I don’t see any test cases covering uppercase in parameter names. I’d like to
see failure case tests.