Prepare WebCore for making the String(const char*) constructor explicit. Making this constructor explicit helps findings cases where a String is constructed from a literal without the ""_s suffix.
Created attachment 455822 [details] Patch
Created attachment 455828 [details] Patch
Created attachment 455830 [details] Patch
Created attachment 455855 [details] Patch
Comment on attachment 455855 [details] Patch r=me
Created attachment 455935 [details] Patch
Comment on attachment 455935 [details] Patch Clearing flags on attachment: 455935 Committed r291992 (248948@trunk): <https://commits.webkit.org/248948@trunk>
All reviewed patches have been landed. Closing bug.
<rdar://problem/90945138>
Reopening to attach new patch.
(In reply to Kate Cheney from comment #10) > Reopening to attach new patch. Sorry, this was a messed up changelog issue. Ignore.
Comment on attachment 455935 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455935&action=review Glad that these are now explicit, but we are making too many temporary String objects! > Source/WebCore/PAL/pal/system/SleepDisabler.h:38 > + static std::unique_ptr<SleepDisabler> create(const String&, Type); Could this have been ASCIILiteral instead of String? > Source/WebCore/dom/Document.cpp:1496 > + return String { PAL::UTF8Encoding().domName() }; I think we can change encoding names to ASCII literals. I also think it’s a mistake to create a new string each time we need the name of UTF-8. This applies many more places. > Source/WebCore/html/MediaFragmentURIParser.cpp:137 > - name = name.utf8(StrictConversion).data(); > + name = String { name.utf8(StrictConversion).data() }; This looks like it was always broken. Converts the name to UTF-8 data and then treats that as Latin-1 and puts it in a WTF::String. Why would that be correct? Maybe this is using WTF::String in a really strange way. > Source/WebCore/html/MediaFragmentURIParser.cpp:141 > - value = value.utf8(StrictConversion).data(); > + value = String { value.utf8(StrictConversion).data() }; Ditto. > Source/WebCore/loader/CrossOriginPreflightResultCache.cpp:88 > + return m_methods.contains(method) || (m_methods.contains("*"_s) && storedCredentialsPolicy != StoredCredentialsPolicy::Use) || isOnAccessControlSimpleRequestMethodAllowlist(method); Bad sign here that we are creating a String out of "*" here so we can compare everything in a vector against it. Not getting worse, but a bad idiom that we should fix. Here and below too. No reason to make a String just to compare with another String.
(In reply to Darin Adler from comment #12) > Comment on attachment 455935 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=455935&action=review > > Glad that these are now explicit, but we are making too many temporary > String objects! > > > Source/WebCore/PAL/pal/system/SleepDisabler.h:38 > > + static std::unique_ptr<SleepDisabler> create(const String&, Type); > > Could this have been ASCIILiteral instead of String? I had tried that at first but internals.createSleepDisabler() allows tests to create a SleepDisabler with any String/reason currently. > > > Source/WebCore/dom/Document.cpp:1496 > > + return String { PAL::UTF8Encoding().domName() }; > > I think we can change encoding names to ASCII literals. I also think it’s a > mistake to create a new string each time we need the name of UTF-8. This > applies many more places. > > > Source/WebCore/html/MediaFragmentURIParser.cpp:137 > > - name = name.utf8(StrictConversion).data(); > > + name = String { name.utf8(StrictConversion).data() }; > > This looks like it was always broken. Converts the name to UTF-8 data and > then treats that as Latin-1 and puts it in a WTF::String. Why would that be > correct? Maybe this is using WTF::String in a really strange way. > > > Source/WebCore/html/MediaFragmentURIParser.cpp:141 > > - value = value.utf8(StrictConversion).data(); > > + value = String { value.utf8(StrictConversion).data() }; > > Ditto. Yes, I had noticed that too but didn't want to cause behavior changes in such a large patch. I have just filed https://bugs.webkit.org/show_bug.cgi?id=238475 to remember to go back to it. > > > Source/WebCore/loader/CrossOriginPreflightResultCache.cpp:88 > > + return m_methods.contains(method) || (m_methods.contains("*"_s) && storedCredentialsPolicy != StoredCredentialsPolicy::Use) || isOnAccessControlSimpleRequestMethodAllowlist(method); > > Bad sign here that we are creating a String out of "*" here so we can > compare everything in a vector against it. Not getting worse, but a bad > idiom that we should fix. Here and below too. No reason to make a String > just to compare with another String.
Comment on attachment 455935 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455935&action=review >>> Source/WebCore/PAL/pal/system/SleepDisabler.h:38 >>> + static std::unique_ptr<SleepDisabler> create(const String&, Type); >> >> Could this have been ASCIILiteral instead of String? > > I had tried that at first but internals.createSleepDisabler() allows tests to create a SleepDisabler with any String/reason currently. Seems sad to have this be less efficient just because of the test harness! Not critical, but likely easy to fix by having the IDL argument be an enum instead of an arbitrary string.
Wrongly reopened.
(In reply to Darin Adler from comment #12) > Comment on attachment 455935 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=455935&action=review > > Glad that these are now explicit, but we are making too many temporary > String objects! > > > Source/WebCore/PAL/pal/system/SleepDisabler.h:38 > > + static std::unique_ptr<SleepDisabler> create(const String&, Type); > > Could this have been ASCIILiteral instead of String? > > > Source/WebCore/dom/Document.cpp:1496 > > + return String { PAL::UTF8Encoding().domName() }; > > I think we can change encoding names to ASCII literals. I also think it’s a > mistake to create a new string each time we need the name of UTF-8. This > applies many more places. > > > Source/WebCore/html/MediaFragmentURIParser.cpp:137 > > - name = name.utf8(StrictConversion).data(); > > + name = String { name.utf8(StrictConversion).data() }; > > This looks like it was always broken. Converts the name to UTF-8 data and > then treats that as Latin-1 and puts it in a WTF::String. Why would that be > correct? Maybe this is using WTF::String in a really strange way. > > > Source/WebCore/html/MediaFragmentURIParser.cpp:141 > > - value = value.utf8(StrictConversion).data(); > > + value = String { value.utf8(StrictConversion).data() }; > > Ditto. > > > Source/WebCore/loader/CrossOriginPreflightResultCache.cpp:88 > > + return m_methods.contains(method) || (m_methods.contains("*"_s) && storedCredentialsPolicy != StoredCredentialsPolicy::Use) || isOnAccessControlSimpleRequestMethodAllowlist(method); > > Bad sign here that we are creating a String out of "*" here so we can > compare everything in a vector against it. Not getting worse, but a bad > idiom that we should fix. Here and below too. No reason to make a String > just to compare with another String. Note that m_methods is not a Vector<String> but a HashSet<String>.
(In reply to Chris Dumez from comment #16) > Note that m_methods is not a Vector<String> but a HashSet<String>. Oh, then we can use the version of contains that takes an HashTranslator. Just need to make the translator to use in this kind of case and put it somewhere.
(In reply to Darin Adler from comment #17) > (In reply to Chris Dumez from comment #16) > > Note that m_methods is not a Vector<String> but a HashSet<String>. > > Oh, then we can use the version of contains that takes an HashTranslator. > Just need to make the translator to use in this kind of case and put it > somewhere. Yes, I am looking into it.
(In reply to Darin Adler from comment #14) > Comment on attachment 455935 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=455935&action=review > > >>> Source/WebCore/PAL/pal/system/SleepDisabler.h:38 > >>> + static std::unique_ptr<SleepDisabler> create(const String&, Type); > >> > >> Could this have been ASCIILiteral instead of String? > > > > I had tried that at first but internals.createSleepDisabler() allows tests to create a SleepDisabler with any String/reason currently. > > Seems sad to have this be less efficient just because of the test harness! > Not critical, but likely easy to fix by having the IDL argument be an enum > instead of an arbitrary string. I looked more into the SleepDisabler issue. It isn't just internals.createSleepDisabler() that needs a String at the moment. When we construct the SleepDisabler from the UIProcess, the reason from the WebContent process via IPC (as a String). Also, the GTK port seems to be using translation for the reason string.
(In reply to Darin Adler from comment #17) > (In reply to Chris Dumez from comment #16) > > Note that m_methods is not a Vector<String> but a HashSet<String>. > > Oh, then we can use the version of contains that takes an HashTranslator. > Just need to make the translator to use in this kind of case and put it > somewhere. Doing so in https://bugs.webkit.org/show_bug.cgi?id=238521.