Bug 136181 – WebCore: Use ASCIILiteral where possible

• Component: WebCore Misc.
• Status: RESOLVED CONFIGURATION CHANGED

[Joseph Pecoraro]

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 *).

[Joseph Pecoraro]

Created attachment 237009 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

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 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 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 on attachment 237009 [details]
[PATCH] Proposed Fix

Based on feedback I'm going to break this up into smaller patches.

[Radar WebKit Bug Importer]

<rdar://problem/47191524>

[Joseph Pecoraro]

This is ancient, and Chris Dumez's string work likely addressed such cases. Closing.