Bug 136181

Summary: WebCore: Use ASCIILiteral where possible
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: WebCore Misc.Assignee: Joseph Pecoraro <joepeck>
Status: RESOLVED CONFIGURATION CHANGED    
Severity: Normal CC: benjamin, commit-queue, darin, ddkilzer, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
[PATCH] Proposed Fix none

Joseph Pecoraro
Reported 2014-08-22 16:33:43 PDT
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 *).
Attachments
[PATCH] Proposed Fix (559.03 KB, patch)
2014-08-22 16:37 PDT, Joseph Pecoraro
no flags
Joseph Pecoraro
Comment 1 2014-08-22 16:37:16 PDT
Created attachment 237009 [details] [PATCH] Proposed Fix
WebKit Commit Bot
Comment 2 2014-08-22 16:39:32 PDT
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.
Darin Adler
Comment 3 2014-08-24 11:56:32 PDT
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 "".
Benjamin Poulain
Comment 4 2014-08-24 22:03:37 PDT
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");
Joseph Pecoraro
Comment 5 2014-08-29 13:51:48 PDT
Comment on attachment 237009 [details] [PATCH] Proposed Fix Based on feedback I'm going to break this up into smaller patches.
Radar WebKit Bug Importer
Comment 6 2019-01-10 14:54:28 PST
Joseph Pecoraro
Comment 7 2022-05-16 14:44:30 PDT
This is ancient, and Chris Dumez's string work likely addressed such cases. Closing.
Note You need to log in before you can comment on or make changes to this bug.