[webkit-reviews] review granted: [Bug 238925] Replace deprecated String(const char*) with String::fromLatin1() in more places : [Attachment 456893] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 238925: Replace deprecated String(const char*) with String::fromLatin1() in
more places
https://bugs.webkit.org/show_bug.cgi?id=238925

Attachment 456893: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 456893
  --> https://bugs.webkit.org/attachment.cgi?id=456893
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.c
pp.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.e
rb: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.