Related with some explanation: <https://webkit.org/b/136179> JavaScriptCore: Use ASCIILiteral where possible My tool only analyzed pre-processed files that the Mac port compiles. So I'm sure other ports / platform code could have changes. I've gone over all the changes manually myself at least once, but I'd appreciate a careful review. I could see us not wanting to make all these changes, or using this as a hint that we should create new methods taking string literals. Like WTFString::find(const char*)/WTFString::contains(const char *).
Created attachment 237009 [details] [PATCH] Proposed Fix
Attachment 237009 [details] did not pass style-queue: ERROR: Source/WebCore/inspector/InspectorHistory.cpp:46: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/html/canvas/WebGLRenderingContext.cpp:6090: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 262 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 237009 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=237009&action=review Sorry, I gave up reviewing this partway through. I think there are some categories here where we’d like to overload functions to take ASCIILiteral instead of String. Sorry, I was confused elsewhere thinking that ASCIILiteral actually compiled in the length, but I now remember that Ben found that bloated code a bit too much for wide use. The thing ASCIILiteral does is tell us that: 1) the string storage is immortal and 2) the characters are all ASCII. For creating a string, (1) is critical. But for other contexts it might be useful to know (2) to disambiguate from UTF-8 strings and even from Latin-1 strings so we can get possibly higher performance implementations. > Source/WTF/wtf/text/WTFString.h:286 > + void append(const char* characters, unsigned length) { append(reinterpret_cast<const LChar*>(characters), length); } Doesn’t seem like we want this; as with other functions in String that take const char* it’s ambiguous whether they are ASCII-only, Latin-1, or UTF-8. Where are we using it exactly? Can we avoid this? I also don’t think that adding this overload optimizes append(ASCIILiteral("abc")), but I think we have code below that is doing that. > Source/WebCore/css/CSSSelectorList.cpp:128 > + result.append(", ", 2); This should be appendLiteral(", "). > Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:184 > + return m_propertySet->propertyIsImportant(propertyID) ? ASCIILiteral("important") : ""; Should be emptyString(), not "".
Comment on attachment 237009 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=237009&action=review We should add HashTable access from StringLiterals, there are too many silly string creation. We should look into having a special constructor of SQLiteStatement that never create a StringImpl for the query. After all, you can use SQLite just fine with literals when using its APIs. Everything I have read is correct...but the patch is waaaayyy too big. This is a wonderful opportunities to find dumb mistakes, we should take advantage of it. Can you please split the patch by subdirectory and submit several patches? > Source/WebCore/inspector/NetworkResourcesData.cpp:168 > + decoder = TextResourceDecoder::create(ASCIILiteral("text/html"), "UTF-8"); "UTF-8" -> TextEncoding::UTF8Encoding() > Source/WebCore/mathml/MathMLMencloseElement.cpp:131 > StringBuilder padding; appendLiteral() below for the "px". This function is so weird. > Source/WebCore/page/ContentSecurityPolicy.cpp:1740 > + String ignoring = ASCIILiteral("The fragment identifier, including the '#', will be ignored."); > if (invalidChar == '?') > - ignoring = "The query component, including the '?', will be ignored."; > + ignoring = ASCIILiteral("The query component, including the '?', will be ignored."); This looks like a bad patter, having if-else for the creation would be better. > Source/WebCore/page/EventSource.cpp:67 > + , m_decoder(TextResourceDecoder::create(ASCIILiteral("text/plain"), "UTF-8")) The "UTF-8" should be TextEncoding::UTF8Encoding() > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1383 > - JSC::Yarr::RegularExpression regExp("webgl_[0123456789abcdefABCDEF]+", TextCaseSensitive); > + JSC::Yarr::RegularExpression regExp(ASCIILiteral("webgl_[0123456789abcdefABCDEF]+"), TextCaseSensitive); That's quite an expensive way of getting substrings! :( > Source/WebCore/platform/network/HTTPParsers.cpp:194 > - if (refresh.find("url", urlStartPos, false) == urlStartPos) { > + if (refresh.find(ASCIILiteral("url"), urlStartPos, false) == urlStartPos) { You should just add an overload for string.find that takes a const char*. > Source/WebCore/platform/network/HTTPParsers.cpp:317 > - pos = mediaType.find("charset", pos, false); > + pos = mediaType.find(ASCIILiteral("charset"), pos, false); Ditto. > Source/WebCore/platform/network/ResourceResponseBase.cpp:407 > - m_cacheControlContainsNoCache = pragmaValue.contains("no-cache", false); > + m_cacheControlContainsNoCache = pragmaValue.contains(ASCIILiteral("no-cache"), false); String::contains() taking a LChar would be good enough for literals. All you need to do is add a version that takes a const char* and does a cast. > Source/WebCore/platform/network/cf/DNSCFNet.cpp:63 > + RetainPtr<CFURLRef> httpCFURL = URL(ParsedURLString, ASCIILiteral("http://example.com/")).createCFURL(); > + RetainPtr<CFURLRef> httpsCFURL = URL(ParsedURLString, ASCIILiteral("https://example.com/")).createCFURL(); This is so bad. I wonder why not creating a CFURL in the first place. The ParsedURLString is okay but that's super shady. > Source/WebCore/testing/Internals.cpp:1994 > result.append(" hotSpot="); appendLiteral() for the ones above and below. > Source/WebCore/workers/WorkerScriptLoader.cpp:128 > + m_decoder = TextResourceDecoder::create(ASCIILiteral("text/javascript"), "UTF-8"); The second argument should be TextEncoding::UTF8Encoding() > Source/WebCore/xml/XMLHttpRequest.cpp:653 > + setRequestHeaderInternal(ASCIILiteral("Content-Type"), ""); "" -> emptyString() > Source/WebCore/xml/XMLHttpRequest.cpp:1162 > - m_decoder = TextResourceDecoder::create("text/html", "UTF-8"); > + m_decoder = TextResourceDecoder::create(ASCIILiteral("text/html"), "UTF-8"); > else > - m_decoder = TextResourceDecoder::create("text/plain", "UTF-8"); > + m_decoder = TextResourceDecoder::create(ASCIILiteral("text/plain"), "UTF-8"); For the second argument, instead of "UTF-8", I think you can pass TextEncoding::UTF8Encoding(). > Source/WebCore/xml/XSLTProcessorLibxslt.cpp:294 > resultType = (const xmlChar*)"html"; It looks like here you could just do return ASCIILiteral("text/html");
Comment on attachment 237009 [details] [PATCH] Proposed Fix Based on feedback I'm going to break this up into smaller patches.
<rdar://problem/47191524>
This is ancient, and Chris Dumez's string work likely addressed such cases. Closing.