[webkit-reviews] review granted: [Bug 238977] Finish porting code base to String::fromLatin1() and make String(const char*) private : [Attachment 457209] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 238977: Finish porting code base to String::fromLatin1() and make
String(const char*) private
https://bugs.webkit.org/show_bug.cgi?id=238977

Attachment 457209: Patch

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




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

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

> Source/WTF/wtf/text/ASCIILiteral.h:73
> +	   return !a;

This should be:

    return false;

I don’t like checking !a again, when the line just above has already checked
it. Or write this:

    if (!a || !b)
	return a == b;

I kind of like that one.

> Source/WTF/wtf/text/ASCIILiteral.h:83
> +    if (!a)
> +	   return !b;
> +    if (!b)
> +	   return !a;
>      return !strcmp(a.characters(), b.characters());

Same applies here. Or we could take advantage of the function above and just
write:

    return a == b.characters();

> Source/WTF/wtf/text/WTFString.h:81
>      WTF_EXPORT_PRIVATE String(const LChar* characters, unsigned length);
>      WTF_EXPORT_PRIVATE String(const char* characters, unsigned length);

I think that later these should be named String::fromLatin1 too.

> Source/WTF/wtf/text/WTFString.h:82
>      ALWAYS_INLINE static String fromLatin1(const char* characters) { return
String { characters }; }

I still like the idea of adding a fromASCII, but I am not pushing you to do
that at this time.

>
Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:88
> +    init.protocol = String::fromUTF8(protocol.get());

Using fromUTF8 adds in the possibility of failure and the need to check for
null, perhaps.

Maybe some day we will want to rename fromUTF8 to have the word "try" in its
name.

> Source/WebCore/Modules/mediastream/gstreamer/GStreamerWebRTCUtils.cpp:159
> +    parameters.rid =
String::fromUTF8(gst_structure_get_string(rtcParameters, "rid"));

Ditto, this now has a new possible failure.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1908
> -	   String str = "WebGL: checkFramebufferStatus:" + String(reason);
> +	   String str = makeString("WebGL: checkFramebufferStatus: ", reason);
>	   printToConsole(MessageLevel::Warning, str);

We should not bother with the local variable here.

> Source/WebCore/loader/soup/ResourceLoaderSoup.cpp:68
> +	   auto contentTypeString = String::fromLatin1(contentType.get());
>	   ResourceResponse response { url,
extractMIMETypeFromMediaType(contentTypeString), static_cast<long
long>(dataSize), extractCharsetFromMediaType(contentTypeString).toString() };

Doesn’t extractMIMETypeFromMediaType take a StringView?

> Source/WebCore/platform/graphics/GLContext.cpp:179
> +	   auto versionString = String::fromLatin1(reinterpret_cast<const
char*>(::glGetString(GL_VERSION)));

This should be changed to use StringView some day. No reason to allocate all
these strings just to parse/split something.

> Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:179
> +	       auto metadata =
String::fromLatin1(gst_element_factory_get_metadata(factory,
GST_ELEMENT_METADATA_KLASS));

This should be changed to use StringView some day. No reason to allocate a
string just to parse/split something.

> Source/WebDriver/WebDriverService.cpp:569
> +	  
completionHandler(CommandResult::fail(CommandResult::ErrorCode::InvalidSessionI
D, String("session deleted because of page crash or hang."_s)));

I bet most of these String() in this file aren’t needed.

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:1177
>	   // DumpRenderTree does not support checking for abandonded
documents.

Spelling error in abandoned here.

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:1178
> +	   CString result("\n");

So strange to use any string at all for this code. Could just have this
instead:

    fputs("Content-Type: text/plain\nContent-Length: 1\n\n#EOF\n", stdout);
    fputs("#EOF\n", stderr);
    fflush(stdout);
    fflush(stderr);

But really not sure if we want to improve this.

> Tools/TestWebKitAPI/Tests/WebCore/PublicSuffix.cpp:182
> +    EXPECT_EQ(String::fromLatin1("åäö"),
topPrivatelyControlledDomain("åäö"_s));
> +    EXPECT_EQ(String::fromLatin1("ÅÄÖ"),
topPrivatelyControlledDomain("ÅÄÖ"_s));

This already was not OK. We should not have any non-ASCII characters in an
ASCIILiteral. In fact, we should be checking that at compile time. If we did,
this would fail to compile.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:285
> +			   auto key = signingParty == TokenSigningParty::Source
? String("source_unlinkable_token"_s) :
String("destination_unlinkable_token"_s);

Should be able to take all the String() out here.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:323
> +					   auto key = signingParty ==
TokenSigningParty::Source ? String("source_secret_token"_s) :
String("destination_secret_token"_s);

And here.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:331
> +					   key = signingParty ==
TokenSigningParty::Source ? String("source_secret_token_signature"_s) :
String("destination_secret_token_signature"_s);

And here.

> Tools/TestWebKitAPI/Tests/mac/ContextMenuCanCopyURL.mm:104
> +    EXPECT_EQ(String("http://www.webkit.org/"_s), String([url
absoluteString]));

Don’t need a String() around the ASCIILiteral, I don’t think.

> Tools/TestWebKitAPI/Tests/mac/WebViewCanPasteURL.mm:49
> +    EXPECT_EQ(String("http://www.webkit.org/"_s), String(text));

Ditto.

> Tools/TestWebKitAPI/Tests/mac/WillPerformClientRedirectToURLCrash.mm:67
> +    EXPECT_EQ(String("PASS"_s), String(message));

Ditto.