Bug 238408 – Prepare WebCore for making the String(const char*) constructor explicit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 455822 [details]
Patch

[Chris Dumez]

Created attachment 455828 [details]
Patch

[Chris Dumez]

Created attachment 455830 [details]
Patch

[Chris Dumez]

Created attachment 455855 [details]
Patch

[Geoffrey Garen]

Comment on attachment 455855 [details]
Patch

r=me

[Chris Dumez]

Created attachment 455935 [details]
Patch

[Chris Dumez]

Comment on attachment 455935 [details]
Patch

Clearing flags on attachment: 455935

Committed r291992 (248948@trunk): <https://commits.webkit.org/248948@trunk>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/90945138>

[Kate Cheney]

Reopening to attach new patch.

[Kate Cheney]

(In reply to Kate Cheney from comment #10)
> Reopening to attach new patch.

Sorry, this was a messed up changelog issue. Ignore.

[Darin Adler]

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.

[Chris Dumez]

(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.

[Darin Adler]

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.

[Chris Dumez]

Wrongly reopened.

[Chris Dumez]

(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>.

[Darin Adler]

(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.

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[Chris Dumez]

(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.