Bug 238977 – Finish porting code base to String::fromLatin1() and make String(const char*) private

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Chris Dumez]

Finish porting code base to String::fromLatin1() and make String(const char*) private.

[Chris Dumez]

Created attachment 457008 [details]
WIP Patch

[Chris Dumez]

Created attachment 457009 [details]
WIP Patch

[Chris Dumez]

Created attachment 457010 [details]
WIP Patch

[Chris Dumez]

Created attachment 457014 [details]
WIP Patch

[Chris Dumez]

Created attachment 457015 [details]
WIP Patch

[Chris Dumez]

Created attachment 457017 [details]
WIP Patch

[Chris Dumez]

Created attachment 457018 [details]
WIP Patch

[Chris Dumez]

Created attachment 457019 [details]
WIP Patch

[Chris Dumez]

Created attachment 457021 [details]
WIP Patch

[Chris Dumez]

Created attachment 457022 [details]
Patch

[Chris Dumez]

Created attachment 457023 [details]
Patch

[Chris Dumez]

Created attachment 457062 [details]
Patch

[Chris Dumez]

Created attachment 457066 [details]
Patch

[Chris Dumez]

Created attachment 457070 [details]
Patch

[Chris Dumez]

Created attachment 457073 [details]
Patch

[Chris Dumez]

Created attachment 457075 [details]
Patch

[Chris Dumez]

Created attachment 457079 [details]
Patch

[Chris Dumez]

Created attachment 457086 [details]
Patch

[Chris Dumez]

Created attachment 457087 [details]
Patch

[Chris Dumez]

Created attachment 457088 [details]
Patch

[Chris Dumez]

Created attachment 457089 [details]
Patch

[Chris Dumez]

Created attachment 457090 [details]
Patch

[Chris Dumez]

Created attachment 457093 [details]
Patch

[Chris Dumez]

Created attachment 457097 [details]
Patch

[Chris Dumez]

Created attachment 457111 [details]
Patch

[Chris Dumez]

Created attachment 457118 [details]
Patch

[Chris Dumez]

Created attachment 457128 [details]
Patch

[Chris Dumez]

Created attachment 457132 [details]
Patch

[Chris Dumez]

Created attachment 457135 [details]
Patch

[Chris Dumez]

Created attachment 457137 [details]
Patch

[Chris Dumez]

Created attachment 457142 [details]
Patch

[Chris Dumez]

Created attachment 457144 [details]
Patch

[Chris Dumez]

Created attachment 457145 [details]
Patch

[Chris Dumez]

Created attachment 457148 [details]
Patch

[Chris Dumez]

Created attachment 457150 [details]
Patch

[Chris Dumez]

Created attachment 457152 [details]
Patch

[Chris Dumez]

Created attachment 457153 [details]
Patch

[Chris Dumez]

Created attachment 457154 [details]
Patch

[Chris Dumez]

Created attachment 457155 [details]
Patch

[Chris Dumez]

Created attachment 457168 [details]
Patch

[Chris Dumez]

Created attachment 457171 [details]
Patch

[Chris Dumez]

Created attachment 457174 [details]
Patch

[Chris Dumez]

Created attachment 457182 [details]
Patch

[Chris Dumez]

Created attachment 457183 [details]
Patch

[Chris Dumez]

Created attachment 457185 [details]
Patch

[Chris Dumez]

Created attachment 457186 [details]
Patch

[Chris Dumez]

Created attachment 457191 [details]
Patch

[Chris Dumez]

Created attachment 457209 [details]
Patch

[Darin Adler]

Comment on attachment 457209 [details]
Patch

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

> Source/WTF/wtf/text/ASCIILiteral.h:73
> +        return !a;

This should be:

    return false;

I don’t like checking !a again, when the line just above has already checked it. Or write this:

    if (!a || !b)
        return a == b;

I kind of like that one.

> Source/WTF/wtf/text/ASCIILiteral.h:83
> +    if (!a)
> +        return !b;
> +    if (!b)
> +        return !a;
>      return !strcmp(a.characters(), b.characters());

Same applies here. Or we could take advantage of the function above and just write:

    return a == b.characters();

> Source/WTF/wtf/text/WTFString.h:81
>      WTF_EXPORT_PRIVATE String(const LChar* characters, unsigned length);
>      WTF_EXPORT_PRIVATE String(const char* characters, unsigned length);

I think that later these should be named String::fromLatin1 too.

> Source/WTF/wtf/text/WTFString.h:82
>      ALWAYS_INLINE static String fromLatin1(const char* characters) { return String { characters }; }

I still like the idea of adding a fromASCII, but I am not pushing you to do that at this time.

> Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:88
> +    init.protocol = String::fromUTF8(protocol.get());

Using fromUTF8 adds in the possibility of failure and the need to check for null, perhaps.

Maybe some day we will want to rename fromUTF8 to have the word "try" in its name.

> Source/WebCore/Modules/mediastream/gstreamer/GStreamerWebRTCUtils.cpp:159
> +    parameters.rid = String::fromUTF8(gst_structure_get_string(rtcParameters, "rid"));

Ditto, this now has a new possible failure.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1908
> -        String str = "WebGL: checkFramebufferStatus:" + String(reason);
> +        String str = makeString("WebGL: checkFramebufferStatus: ", reason);
>          printToConsole(MessageLevel::Warning, str);

We should not bother with the local variable here.

> Source/WebCore/loader/soup/ResourceLoaderSoup.cpp:68
> +        auto contentTypeString = String::fromLatin1(contentType.get());
>          ResourceResponse response { url, extractMIMETypeFromMediaType(contentTypeString), static_cast<long long>(dataSize), extractCharsetFromMediaType(contentTypeString).toString() };

Doesn’t extractMIMETypeFromMediaType take a StringView?

> Source/WebCore/platform/graphics/GLContext.cpp:179
> +        auto versionString = String::fromLatin1(reinterpret_cast<const char*>(::glGetString(GL_VERSION)));

This should be changed to use StringView some day. No reason to allocate all these strings just to parse/split something.

> Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:179
> +            auto metadata = String::fromLatin1(gst_element_factory_get_metadata(factory, GST_ELEMENT_METADATA_KLASS));

This should be changed to use StringView some day. No reason to allocate a string just to parse/split something.

> Source/WebDriver/WebDriverService.cpp:569
> +        completionHandler(CommandResult::fail(CommandResult::ErrorCode::InvalidSessionID, String("session deleted because of page crash or hang."_s)));

I bet most of these String() in this file aren’t needed.

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:1177
>          // DumpRenderTree does not support checking for abandonded documents.

Spelling error in abandoned here.

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:1178
> +        CString result("\n");

So strange to use any string at all for this code. Could just have this instead:

    fputs("Content-Type: text/plain\nContent-Length: 1\n\n#EOF\n", stdout);
    fputs("#EOF\n", stderr);
    fflush(stdout);
    fflush(stderr);

But really not sure if we want to improve this.

> Tools/TestWebKitAPI/Tests/WebCore/PublicSuffix.cpp:182
> +    EXPECT_EQ(String::fromLatin1("åäö"), topPrivatelyControlledDomain("åäö"_s));
> +    EXPECT_EQ(String::fromLatin1("ÅÄÖ"), topPrivatelyControlledDomain("ÅÄÖ"_s));

This already was not OK. We should not have any non-ASCII characters in an ASCIILiteral. In fact, we should be checking that at compile time. If we did, this would fail to compile.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:285
> +                        auto key = signingParty == TokenSigningParty::Source ? String("source_unlinkable_token"_s) : String("destination_unlinkable_token"_s);

Should be able to take all the String() out here.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:323
> +                                        auto key = signingParty == TokenSigningParty::Source ? String("source_secret_token"_s) : String("destination_secret_token"_s);

And here.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:331
> +                                        key = signingParty == TokenSigningParty::Source ? String("source_secret_token_signature"_s) : String("destination_secret_token_signature"_s);

And here.

> Tools/TestWebKitAPI/Tests/mac/ContextMenuCanCopyURL.mm:104
> +    EXPECT_EQ(String("http://www.webkit.org/"_s), String([url absoluteString]));

Don’t need a String() around the ASCIILiteral, I don’t think.

> Tools/TestWebKitAPI/Tests/mac/WebViewCanPasteURL.mm:49
> +    EXPECT_EQ(String("http://www.webkit.org/"_s), String(text));

Ditto.

> Tools/TestWebKitAPI/Tests/mac/WillPerformClientRedirectToURLCrash.mm:67
> +    EXPECT_EQ(String("PASS"_s), String(message));

Ditto.

[Chris Dumez]

Comment on attachment 457209 [details]
Patch

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

>> Source/WTF/wtf/text/ASCIILiteral.h:73
>> +        return !a;
> 
> This should be:
> 
>     return false;
> 
> I don’t like checking !a again, when the line just above has already checked it. Or write this:
> 
>     if (!a || !b)
>         return a == b;
> 
> I kind of like that one.

Ok, will change. I think I'll have to use this though:
if (!a || !b)
    return a.characters() == b;

FYI, I had copied the logic from String's operator==.

>> Source/WTF/wtf/text/WTFString.h:81
>>      WTF_EXPORT_PRIVATE String(const char* characters, unsigned length);
> 
> I think that later these should be named String::fromLatin1 too.

I agree.

>> Source/WTF/wtf/text/WTFString.h:82
>>      ALWAYS_INLINE static String fromLatin1(const char* characters) { return String { characters }; }
> 
> I still like the idea of adding a fromASCII, but I am not pushing you to do that at this time.

I don't fully understand the benefit of the fromASCII(). Sure, we can artificially enforce via assertions that the input is ASCII but it won't really help us write a more efficient constructor, will it?
If so, is it really worth adding yet another way to construct a String?

>> Source/WebCore/loader/soup/ResourceLoaderSoup.cpp:68
>>          ResourceResponse response { url, extractMIMETypeFromMediaType(contentTypeString), static_cast<long long>(dataSize), extractCharsetFromMediaType(contentTypeString).toString() };
> 
> Doesn’t extractMIMETypeFromMediaType take a StringView?

No, not at the moment. It could but it seems there is a potential optimization where it returned the String passed in, as-is.

>> Tools/TestWebKitAPI/Tests/WebCore/PublicSuffix.cpp:182
>> +    EXPECT_EQ(String::fromLatin1("ÅÄÖ"), topPrivatelyControlledDomain("ÅÄÖ"_s));
> 
> This already was not OK. We should not have any non-ASCII characters in an ASCIILiteral. In fact, we should be checking that at compile time. If we did, this would fail to compile.

Weird, we do have assertions in place already:
```
constexpr ASCIILiteral operator"" _s(const char* characters, size_t n)
{
#if ASSERT_ENABLED
    for (size_t i = 0; i < n; ++i)
        ASSERT_UNDER_CONSTEXPR_CONTEXT(isASCII(characters[i]));
#else
    UNUSED_PARAM(n);
#endif
    return ASCIILiteral::fromLiteralUnsafe(characters);
}
```

[Chris Dumez]

(In reply to Chris Dumez from comment #50)
> Comment on attachment 457209 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457209&action=review
> 
> >> Source/WTF/wtf/text/ASCIILiteral.h:73
> >> +        return !a;
> > 
> > This should be:
> > 
> >     return false;
> > 
> > I don’t like checking !a again, when the line just above has already checked it. Or write this:
> > 
> >     if (!a || !b)
> >         return a == b;
> > 
> > I kind of like that one.
> 
> Ok, will change. I think I'll have to use this though:
> if (!a || !b)
>     return a.characters() == b;
> 
> FYI, I had copied the logic from String's operator==.
> 
> >> Source/WTF/wtf/text/WTFString.h:81
> >>      WTF_EXPORT_PRIVATE String(const char* characters, unsigned length);
> > 
> > I think that later these should be named String::fromLatin1 too.
> 
> I agree.
> 
> >> Source/WTF/wtf/text/WTFString.h:82
> >>      ALWAYS_INLINE static String fromLatin1(const char* characters) { return String { characters }; }
> > 
> > I still like the idea of adding a fromASCII, but I am not pushing you to do that at this time.
> 
> I don't fully understand the benefit of the fromASCII(). Sure, we can
> artificially enforce via assertions that the input is ASCII but it won't
> really help us write a more efficient constructor, will it?
> If so, is it really worth adding yet another way to construct a String?
> 
> >> Source/WebCore/loader/soup/ResourceLoaderSoup.cpp:68
> >>          ResourceResponse response { url, extractMIMETypeFromMediaType(contentTypeString), static_cast<long long>(dataSize), extractCharsetFromMediaType(contentTypeString).toString() };
> > 
> > Doesn’t extractMIMETypeFromMediaType take a StringView?
> 
> No, not at the moment. It could but it seems there is a potential
> optimization where it returned the String passed in, as-is.
> 
> >> Tools/TestWebKitAPI/Tests/WebCore/PublicSuffix.cpp:182
> >> +    EXPECT_EQ(String::fromLatin1("ÅÄÖ"), topPrivatelyControlledDomain("ÅÄÖ"_s));
> > 
> > This already was not OK. We should not have any non-ASCII characters in an ASCIILiteral. In fact, we should be checking that at compile time. If we did, this would fail to compile.
> 
> Weird, we do have assertions in place already:
> ```
> constexpr ASCIILiteral operator"" _s(const char* characters, size_t n)
> {
> #if ASSERT_ENABLED
>     for (size_t i = 0; i < n; ++i)
>         ASSERT_UNDER_CONSTEXPR_CONTEXT(isASCII(characters[i]));
> #else
>     UNUSED_PARAM(n);
> #endif
>     return ASCIILiteral::fromLiteralUnsafe(characters);
> }
> ```

Oh, the assertion does hit:
ASSERTION FAILED: isASCII(characters[i])
/Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/text/ASCIILiteral.h(88) : WTF::ASCIILiteral WTF::StringLiterals::operator""_s(const char *, size_t)
worker/0 TestWebKitAPI.PublicSuffix.TopPrivatelyControlledDomain Crashed
Ran 1 tests of 1 with 0 successful
------------------------------
Test suite failed

Crashed

    TestWebKitAPI.PublicSuffix.TopPrivatelyControlledDomain
        ASSERTION FAILED: isASCII(characters[i])
        /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/text/ASCIILiteral.h(88) : WTF::ASCIILiteral WTF::StringLiterals::operator""_s(const char *, size_t)

[Chris Dumez]

Created attachment 457216 [details]
Patch

[Chris Dumez]

Comment on attachment 457216 [details]
Patch

Clearing flags on attachment: 457216

Committed r292696 (?): <https://commits.webkit.org/r292696>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/91546913>

[EWS]

Found 2 new test failures: fast/multicol/client-rects-spanners.html, fast/multicol/span/span-as-immediate-child-generated-content.html

[Darin Adler]

Comment on attachment 457209 [details]
Patch

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

>>> Source/WTF/wtf/text/WTFString.h:82
>>>      ALWAYS_INLINE static String fromLatin1(const char* characters) { return String { characters }; }
>> 
>> I still like the idea of adding a fromASCII, but I am not pushing you to do that at this time.
> 
> I don't fully understand the benefit of the fromASCII(). Sure, we can artificially enforce via assertions that the input is ASCII but it won't really help us write a more efficient constructor, will it?
> If so, is it really worth adding yet another way to construct a String?

Not critical. Maybe we should discuss this in person at some point. I still like the idea and I think it is valuable for two reasons, but I don’t want to write more paragraphs about why. The two reasons are: (1) being explicit about what we intend at each call site, instead of writing fromLatin1 because it "just doesn’t matter it’s always ASCII", and (2) future-proofing these call sites a little if we want to do experiments with storing UTF-8.