Related: <https://webkit.org/b/136179> JavaScriptCore: Use ASCIILiteral where possible Final batch of automated ASCIILiteral insertion.
Created attachment 237010 [details] [PATCH] Proposed Fix
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?
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.
<rdar://problem/47191485>