Bug 238925 – Replace deprecated String(const char*) with String::fromLatin1() in more places

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Chris Dumez]

Replace deprecated String(const char*) with String::fromLatin1() in more places.

[Chris Dumez]

Created attachment 456893 [details]
Patch

[Darin Adler]

Comment on attachment 456893 [details]
Patch

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

I am puzzled by many of these, but grepping for String::fromLatin1 later will be easy so this is a step in the right direction.

Sadly, grepping for const char* used in makeString, implicitly treated as Latin-1, is not so easy. So maybe later we could tighten makeString as well, and requiring that we specify which const char* are Latin-1 vs. UTF-8 vs. ASCII vs. ASCII literal.

> Source/WebKit/NetworkProcess/DatabaseUtilities.cpp:208
> +    return String::fromLatin1(originalQuery).replace("CREATE UNIQUE INDEX IF NOT EXISTS", "CREATE UNIQUE INDEX");

I love this.

Should we also make a String::fromASCII that is the same as Latin-1, but has a debug-only assertion that there are no non-ASCII characters? If we had that, we would probably use that here. It seems like there are a lot of call sites that use String::fromLatin1, not because it’s a Latin-1 string, but because it’s a string where all the characters are ASCII, and I would love the clarity from saying what we mean in such cases. This would be a debug-only check, so not super-risky.

Also if we some day we change String representation to use UTF-8, then the distinction might matter.

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:611
> +    CString prologue { "{\n\"entries\": [\n" };
>      writeToFile(fd, prologue.data(), prologue.length());

Let’s get rid of the unnecessary memory allocation with something more like this:

    const char* prologue = "{\n\"entries\": [\n";
    writeToFile(fd, prologue, std::strlen(prologue));

> Source/WebKit/Scripts/PreferencesTemplates/WebPreferencesExperimentalFeatures.cpp.erb:44
> +        API::ExperimentalFeature::create(String::fromLatin1(<%= @pref.humanReadableName %>), "<%= @pref.name %>"_s, String::fromLatin1(<%= @pref.humanReadableDescription %>), DEFAULT_VALUE_FOR_<%= @pref.name %>, <%= @pref.hidden %>),

These names aren’t known at compile time?

> Source/WebKit/Scripts/PreferencesTemplates/WebPreferencesInternalDebugFeatures.cpp.erb:43
> +        API::InternalDebugFeature::create(String::fromLatin1(<%= @pref.humanReadableName %>), "<%= @pref.name %>"_s, String::fromLatin1(<%= @pref.humanReadableDescription %>), DEFAULT_VALUE_FOR_<%= @pref.name %>, <%= @pref.hidden %>),

Same question.

> Source/WebKit/Scripts/PreferencesTemplates/WebPreferencesStoreDefaultsMap.cpp.erb:56
> +        defaults.get().set(WebPreferencesKey::<%= @pref.nameLower %>Key(), Value(makeString(DEFAULT_VALUE_FOR_<%= @pref.name %>)));

What exactly are we converting to a String here? Just curious.

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:302
> +    return { { WTFMove(handle), String::fromLatin1(path.data()) } };

This one seems clearly wrong. Why wouldn’t we use pathString here, which is correctly made with String::fromUTF8. Instead the old code incorrectly makes a second String with the wrong encoding!

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1903
> +    showedWarningDictionary.set("source"_s, "service"_str);

How did you know it was a good idea to use _str instead of _s here? Part of why I ask is that Geoff was proposing we remove _str at some point; adding new uses does not help us move in that direction.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1918
> +        dictionary.set("source"_s, "service"_str);

And here.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1935
> +        dictionary.set("action"_s, "go back"_s);

But not here.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:192
> +        (id)kSecAttrAccessGroup: (id)@(LocalAuthenticatorAccessGroup),

I don’t think we need (id) before the @.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:173
> +        (id)kSecAttrAccessGroup: (id)@(LocalAuthenticatorAccessGroup),

Ditto.

[Chris Dumez]

(In reply to Darin Adler from comment #2)
> Comment on attachment 456893 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456893&action=review
> 
> I am puzzled by many of these, but grepping for String::fromLatin1 later
> will be easy so this is a step in the right direction.
> 
> Sadly, grepping for const char* used in makeString, implicitly treated as
> Latin-1, is not so easy. So maybe later we could tighten makeString as well,
> and requiring that we specify which const char* are Latin-1 vs. UTF-8 vs.
> ASCII vs. ASCII literal.
> 
> > Source/WebKit/NetworkProcess/DatabaseUtilities.cpp:208
> > +    return String::fromLatin1(originalQuery).replace("CREATE UNIQUE INDEX IF NOT EXISTS", "CREATE UNIQUE INDEX");
> 
> I love this.
> 
> Should we also make a String::fromASCII that is the same as Latin-1, but has
> a debug-only assertion that there are no non-ASCII characters? If we had
> that, we would probably use that here. It seems like there are a lot of call
> sites that use String::fromLatin1, not because it’s a Latin-1 string, but
> because it’s a string where all the characters are ASCII, and I would love
> the clarity from saying what we mean in such cases. This would be a
> debug-only check, so not super-risky.
> 
> Also if we some day we change String representation to use UTF-8, then the
> distinction might matter.
> 
> > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:611
> > +    CString prologue { "{\n\"entries\": [\n" };
> >      writeToFile(fd, prologue.data(), prologue.length());
> 
> Let’s get rid of the unnecessary memory allocation with something more like
> this:
> 
>     const char* prologue = "{\n\"entries\": [\n";
>     writeToFile(fd, prologue, std::strlen(prologue));
> 
> > Source/WebKit/Scripts/PreferencesTemplates/WebPreferencesExperimentalFeatures.cpp.erb:44
> > +        API::ExperimentalFeature::create(String::fromLatin1(<%= @pref.humanReadableName %>), "<%= @pref.name %>"_s, String::fromLatin1(<%= @pref.humanReadableDescription %>), DEFAULT_VALUE_FOR_<%= @pref.name %>, <%= @pref.hidden %>),
> 
> These names aren’t known at compile time?
> 
> > Source/WebKit/Scripts/PreferencesTemplates/WebPreferencesInternalDebugFeatures.cpp.erb:43
> > +        API::InternalDebugFeature::create(String::fromLatin1(<%= @pref.humanReadableName %>), "<%= @pref.name %>"_s, String::fromLatin1(<%= @pref.humanReadableDescription %>), DEFAULT_VALUE_FOR_<%= @pref.name %>, <%= @pref.hidden %>),
> 
> Same question.
> 
> > Source/WebKit/Scripts/PreferencesTemplates/WebPreferencesStoreDefaultsMap.cpp.erb:56
> > +        defaults.get().set(WebPreferencesKey::<%= @pref.nameLower %>Key(), Value(makeString(DEFAULT_VALUE_FOR_<%= @pref.name %>)));
> 
> What exactly are we converting to a String here? Just curious.

I struggled a lot yesterday getting things building. Basically, the default value is defined in the yaml file. It usually looks like: '"foo"', '{}', 'defaultValueForFoo'.
String() didn't work because you cannot you need to call fromLatin1() when the input is a const char*. If I remember correctly, fromLatin1() didn't work either because something the input is not a const char* (could be a String for e.g.).
I also tried to define all the string values in the yaml files as either ASCIILiteral or String() but that didn't work because one of our generated Cocoa files is "boxing" the default value with @().

[Chris Dumez]

(In reply to Darin Adler from comment #2)
> Comment on attachment 456893 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456893&action=review
> 
> I am puzzled by many of these, but grepping for String::fromLatin1 later
> will be easy so this is a step in the right direction.
> 
> Sadly, grepping for const char* used in makeString, implicitly treated as
> Latin-1, is not so easy. So maybe later we could tighten makeString as well,
> and requiring that we specify which const char* are Latin-1 vs. UTF-8 vs.
> ASCII vs. ASCII literal.
> 
> > Source/WebKit/NetworkProcess/DatabaseUtilities.cpp:208
> > +    return String::fromLatin1(originalQuery).replace("CREATE UNIQUE INDEX IF NOT EXISTS", "CREATE UNIQUE INDEX");
> 
> I love this.
> 
> Should we also make a String::fromASCII that is the same as Latin-1, but has
> a debug-only assertion that there are no non-ASCII characters? If we had
> that, we would probably use that here. It seems like there are a lot of call
> sites that use String::fromLatin1, not because it’s a Latin-1 string, but
> because it’s a string where all the characters are ASCII, and I would love
> the clarity from saying what we mean in such cases. This would be a
> debug-only check, so not super-risky.
> 
> Also if we some day we change String representation to use UTF-8, then the
> distinction might matter.
> 
> > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:611
> > +    CString prologue { "{\n\"entries\": [\n" };
> >      writeToFile(fd, prologue.data(), prologue.length());
> 
> Let’s get rid of the unnecessary memory allocation with something more like
> this:
> 
>     const char* prologue = "{\n\"entries\": [\n";
>     writeToFile(fd, prologue, std::strlen(prologue));
> 
> > Source/WebKit/Scripts/PreferencesTemplates/WebPreferencesExperimentalFeatures.cpp.erb:44
> > +        API::ExperimentalFeature::create(String::fromLatin1(<%= @pref.humanReadableName %>), "<%= @pref.name %>"_s, String::fromLatin1(<%= @pref.humanReadableDescription %>), DEFAULT_VALUE_FOR_<%= @pref.name %>, <%= @pref.hidden %>),
> 
> These names aren’t known at compile time?
> 
> > Source/WebKit/Scripts/PreferencesTemplates/WebPreferencesInternalDebugFeatures.cpp.erb:43
> > +        API::InternalDebugFeature::create(String::fromLatin1(<%= @pref.humanReadableName %>), "<%= @pref.name %>"_s, String::fromLatin1(<%= @pref.humanReadableDescription %>), DEFAULT_VALUE_FOR_<%= @pref.name %>, <%= @pref.hidden %>),
> 
> Same question.
> 
> > Source/WebKit/Scripts/PreferencesTemplates/WebPreferencesStoreDefaultsMap.cpp.erb:56
> > +        defaults.get().set(WebPreferencesKey::<%= @pref.nameLower %>Key(), Value(makeString(DEFAULT_VALUE_FOR_<%= @pref.name %>)));
> 
> What exactly are we converting to a String here? Just curious.
> 
> > Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:302
> > +    return { { WTFMove(handle), String::fromLatin1(path.data()) } };
> 
> This one seems clearly wrong. Why wouldn’t we use pathString here, which is
> correctly made with String::fromUTF8. Instead the old code incorrectly makes
> a second String with the wrong encoding!
> 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1903
> > +    showedWarningDictionary.set("source"_s, "service"_str);

I'll double check if _s would have built. I decided to be explicit about it being a String because the value in the map is a variant which may be a String (or other things).

I personally don't think it is worth removing _str, it helps in some very specific cases write a bit more concise code by letting the compiler know this is a String (and not an ASCIILiteral which could be converted to a String or something else).

[Chris Dumez]

(In reply to Darin Adler from comment #2)
> Comment on attachment 456893 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456893&action=review
> 
> I am puzzled by many of these, but grepping for String::fromLatin1 later
> will be easy so this is a step in the right direction.
> 
> Sadly, grepping for const char* used in makeString, implicitly treated as
> Latin-1, is not so easy. So maybe later we could tighten makeString as well,
> and requiring that we specify which const char* are Latin-1 vs. UTF-8 vs.
> ASCII vs. ASCII literal.
> 
> > Source/WebKit/NetworkProcess/DatabaseUtilities.cpp:208
> > +    return String::fromLatin1(originalQuery).replace("CREATE UNIQUE INDEX IF NOT EXISTS", "CREATE UNIQUE INDEX");
> 
> I love this.
> 
> Should we also make a String::fromASCII that is the same as Latin-1, but has
> a debug-only assertion that there are no non-ASCII characters? If we had
> that, we would probably use that here. It seems like there are a lot of call
> sites that use String::fromLatin1, not because it’s a Latin-1 string, but
> because it’s a string where all the characters are ASCII, and I would love
> the clarity from saying what we mean in such cases. This would be a
> debug-only check, so not super-risky.
> 
> Also if we some day we change String representation to use UTF-8, then the
> distinction might matter.
> 
> > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:611
> > +    CString prologue { "{\n\"entries\": [\n" };
> >      writeToFile(fd, prologue.data(), prologue.length());
> 
> Let’s get rid of the unnecessary memory allocation with something more like
> this:
> 
>     const char* prologue = "{\n\"entries\": [\n";
>     writeToFile(fd, prologue, std::strlen(prologue));
> 
> > Source/WebKit/Scripts/PreferencesTemplates/WebPreferencesExperimentalFeatures.cpp.erb:44
> > +        API::ExperimentalFeature::create(String::fromLatin1(<%= @pref.humanReadableName %>), "<%= @pref.name %>"_s, String::fromLatin1(<%= @pref.humanReadableDescription %>), DEFAULT_VALUE_FOR_<%= @pref.name %>, <%= @pref.hidden %>),
> 
> These names aren’t known at compile time?

Yes, I believe they are know at compile time and defined as const char* ("foo") in the YAML files.
I'll double check but I am pretty sure I tried using ""_s or ""_str in the yaml files but it failed building because there is a Cocoa file "boxing" these with @() in ObjC.

[Chris Dumez]

Created attachment 456961 [details]
Patch

[Darin Adler]

(In reply to Chris Dumez from comment #5)
> > > +        API::ExperimentalFeature::create(String::fromLatin1(<%= @pref.humanReadableName %>), "<%= @pref.name %>"_s, String::fromLatin1(<%= @pref.humanReadableDescription %>), DEFAULT_VALUE_FOR_<%= @pref.name %>, <%= @pref.hidden %>),
> > 
> > These names aren’t known at compile time?
> 
> Yes, I believe they are know at compile time and defined as const char*
> ("foo") in the YAML files.
> I'll double check but I am pretty sure I tried using ""_s or ""_str in the
> yaml files but it failed building because there is a Cocoa file "boxing"
> these with @() in ObjC.

If we know it’s a literal this should work:

    <%= @pref.humanReadableName %> ""_s

Rather than:

    String::fromLatin1(<%= @pref.humanReadableName %>)

Token pasting should combine the string in in the <% %> with the ""_s, producing an ASCIILiteral.

[Chris Dumez]

(In reply to Darin Adler from comment #7)
> (In reply to Chris Dumez from comment #5)
> > > > +        API::ExperimentalFeature::create(String::fromLatin1(<%= @pref.humanReadableName %>), "<%= @pref.name %>"_s, String::fromLatin1(<%= @pref.humanReadableDescription %>), DEFAULT_VALUE_FOR_<%= @pref.name %>, <%= @pref.hidden %>),
> > > 
> > > These names aren’t known at compile time?
> > 
> > Yes, I believe they are know at compile time and defined as const char*
> > ("foo") in the YAML files.
> > I'll double check but I am pretty sure I tried using ""_s or ""_str in the
> > yaml files but it failed building because there is a Cocoa file "boxing"
> > these with @() in ObjC.
> 
> If we know it’s a literal this should work:
> 
>     <%= @pref.humanReadableName %> ""_s

Ok, I can try that.

FYI, I had tried:
<%= @pref.humanReadableName %>_s

but that didn't work because one of the human names was a #define.

[Chris Dumez]

Created attachment 456978 [details]
Patch

[Chris Dumez]

Comment on attachment 456978 [details]
Patch

Clearing flags on attachment: 456978

Committed r292587 (249420@trunk): <https://commits.webkit.org/249420@trunk>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/91458997>