Bug 136183 – WebKit and WebKit2: Use ASCIILiteral where possible

• Component: WebKit2
• Status: NEW

[Joseph Pecoraro]

Related: <https://webkit.org/b/136179> JavaScriptCore: Use ASCIILiteral where possible

Final batch of automated ASCIILiteral insertion.

[Joseph Pecoraro]

Created attachment 237010 [details]
[PATCH] Proposed Fix

[Benjamin Poulain]

Comment on attachment 237010 [details]
[PATCH] Proposed Fix

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

> Source/WebKit/cf/WebCoreSupport/WebInspectorClientCF.cpp:84
> -        *setting = static_cast<bool>(CFBooleanGetValue(static_cast<CFBooleanRef>(value.get()))) ? "true" : "false";
> +        *setting = static_cast<bool>(CFBooleanGetValue(static_cast<CFBooleanRef>(value.get()))) ? ASCIILiteral("true") : ASCIILiteral("false");

From how this is used, it looks like populateSetting should return a TriState and the call site should create the string as needed.

> Source/WebKit/cf/WebCoreSupport/WebInspectorClientCF.cpp:86
>          *setting = "";

Unrelated but that looks like a bad use of strings. When a value does not exist, the return should be a null string, not an empty string.

> Source/WebKit2/Shared/mac/ChildProcessMac.mm:143
>      String path = String::fromUTF8(pwd.pw_dir);
> -    path.append("/Library");
> +    path.append(ASCIILiteral("/Library"));

String path = String::fromUTF8(pwd.pw_dir) + "/Library";

> Source/WebKit2/Shared/mac/ChildProcessMac.mm:147
> -    path.append("/Preferences");
> +    path.append(ASCIILiteral("/Preferences"));

Let's use StringOperators here, no need to create a new StringImpl for "/Preferences"

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:57
>      case NSLeftTextAlignment:
> -        return "left";
> +        return ASCIILiteral("left");

I hope clang only generate one constructor here.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:144
> +    map->add(ASCIILiteral("insertNewlineIgnoringFieldEditor:"), "InsertNewline");
> +    map->add(ASCIILiteral("insertParagraphSeparator:"), "InsertNewline");
> +    map->add(ASCIILiteral("insertTabIgnoringFieldEditor:"), "InsertTab");
> +    map->add(ASCIILiteral("pageDown:"), "MovePageDown");
> +    map->add(ASCIILiteral("pageDownAndModifySelection:"), "MovePageDownAndModifySelection");
> +    map->add(ASCIILiteral("pageUp:"), "MovePageUp");
> +    map->add(ASCIILiteral("pageUpAndModifySelection:"), "MovePageUpAndModifySelection");

Why only the first of the two strings?

[Darin Adler]

Comment on attachment 237010 [details]
[PATCH] Proposed Fix

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

Seems like some of this code may be overusing String where we could use string literals. The problem is we have no better type for a string literal than "const char*" and that type does not pass the length through nor does it communicate the fact that the entire string is ASCII. And also if do hard code something to be a string literal than it’s troublesome if we want to compute its value in one case.

>> Source/WebKit/cf/WebCoreSupport/WebInspectorClientCF.cpp:86
>>          *setting = "";
> 
> Unrelated but that looks like a bad use of strings. When a value does not exist, the return should be a null string, not an empty string.

Maybe, but I don’t think that’s universally true. Sometimes the handy value to use for "not existing" is a null string, other times it’s better to always return a non-null string. I don’t think there’s a clear rule of thumb. Of course, it’s more efficient to use emptyString() rather than "" for an empty WTF::String.

>> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:57
>> +        return ASCIILiteral("left");
> 
> I hope clang only generate one constructor here.

Or the return value from this could just be const char* instead of const String.

[Radar WebKit Bug Importer]

<rdar://problem/47191485>