Finish porting code base to String::fromLatin1() and make String(const char*) private.
Created attachment 457008 [details] WIP Patch
Created attachment 457009 [details] WIP Patch
Created attachment 457010 [details] WIP Patch
Created attachment 457014 [details] WIP Patch
Created attachment 457015 [details] WIP Patch
Created attachment 457017 [details] WIP Patch
Created attachment 457018 [details] WIP Patch
Created attachment 457019 [details] WIP Patch
Created attachment 457021 [details] WIP Patch
Created attachment 457022 [details] Patch
Created attachment 457023 [details] Patch
Created attachment 457062 [details] Patch
Created attachment 457066 [details] Patch
Created attachment 457070 [details] Patch
Created attachment 457073 [details] Patch
Created attachment 457075 [details] Patch
Created attachment 457079 [details] Patch
Created attachment 457086 [details] Patch
Created attachment 457087 [details] Patch
Created attachment 457088 [details] Patch
Created attachment 457089 [details] Patch
Created attachment 457090 [details] Patch
Created attachment 457093 [details] Patch
Created attachment 457097 [details] Patch
Created attachment 457111 [details] Patch
Created attachment 457118 [details] Patch
Created attachment 457128 [details] Patch
Created attachment 457132 [details] Patch
Created attachment 457135 [details] Patch
Created attachment 457137 [details] Patch
Created attachment 457142 [details] Patch
Created attachment 457144 [details] Patch
Created attachment 457145 [details] Patch
Created attachment 457148 [details] Patch
Created attachment 457150 [details] Patch
Created attachment 457152 [details] Patch
Created attachment 457153 [details] Patch
Created attachment 457154 [details] Patch
Created attachment 457155 [details] Patch
Created attachment 457168 [details] Patch
Created attachment 457171 [details] Patch
Created attachment 457174 [details] Patch
Created attachment 457182 [details] Patch
Created attachment 457183 [details] Patch
Created attachment 457185 [details] Patch
Created attachment 457186 [details] Patch
Created attachment 457191 [details] Patch
Created attachment 457209 [details] Patch
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.
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); } ```
(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)
Created attachment 457216 [details] Patch
Comment on attachment 457216 [details] Patch Clearing flags on attachment: 457216 Committed r292696 (?): <https://commits.webkit.org/r292696>
All reviewed patches have been landed. Closing bug.
<rdar://problem/91546913>
Found 2 new test failures: fast/multicol/client-rects-spanners.html, fast/multicol/span/span-as-immediate-child-generated-content.html
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.