[webkit-reviews] review granted: [Bug 238525] Prepare WebKit/ & WebKitLegacy/ for making the String(const char*) constructor explicit : [Attachment 456179] Patch

Wed Mar 30 19:57:36 PDT 2022

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 238525: Prepare WebKit/ & WebKitLegacy/ for making the String(const char*)
constructor explicit
https://bugs.webkit.org/show_bug.cgi?id=238525

Attachment 456179: Patch

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

--- Comment #16 from Darin Adler <darin at apple.com> ---
Comment on attachment 456179
  --> https://bugs.webkit.org/attachment.cgi?id=456179
Patch

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

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:253
> -    String tableStatement = database.tableSQL("Records"_s);
> +    String tableStatement = database.tableSQL("Records");

I’ll just say this once and not push too hard: I would have liked to be able to
use ASCIILiteral instead of const char* here, just so the rule about when to
use _s was simpler. I’m thinking that it should almost never be harmful to
*add* _s; gives a little more information that this is a literal, guaranteed
all-ASCII, and has a constant-foldable strlen that we could choose to take
advantage of in future, and also allows converting to const char* so doesn’t
require much repetition of code if we don’t want to optimize right now. For
example, maybe here the specific thing needed is a StringView constructor that
takes an ASCIILiteral?

Generally it seems this should be pretty easy, adding various overloads that
take ASCIILiteral; only tricky thing is if we create some new ambiguity that
causes us to have to touch other call sites.

If we pursued the strategy above, then there would be no change to this line of
code, and quite a few others in this patch, where we would not need to remove
_s.

>
Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDat
abase.cpp:716
> +    constexpr ASCIILiteral attributedTableName =
"AttributedPrivateClickMeasurement"_s;

Why not auto here?

> Source/WebKit/NetworkProcess/cache/NetworkCacheBlobStorage.cpp:126
> -    auto linkPath = FileSystem::fileSystemRepresentation(path);
> -    auto data = mapFile(linkPath.data());
> +    auto data = mapFile(path);

Seems like this change might *fix* a bug for non-ASCII characters in Blob
storage paths, since it will no longer convert to UTF-8 (file system
representation) and then convert back to a WTF::String, treating the non-ASCII
characters as Latin-1, not UTF-8.

> Source/WebKit/NetworkProcess/cache/NetworkCacheData.cpp:-69
> -    return mapFile(FileSystem::fileSystemRepresentation(path).data());

Same here.

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

Does this function need to take String rather than ASCIILiteral?

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

Does this function need to take String rather than ASCIILiteral?

> Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm:230
> -    return [targetMethodSignature
_signatureForBlockAtArgumentIndex:blockIndex]._typeString.UTF8String;
> +    return [targetMethodSignature
_signatureForBlockAtArgumentIndex:blockIndex]._typeString;

If there could ever have been non-ASCII characters here, then this fixed
another bug where we do the convert to UTF-8 and then treat as Latin-1 thing.

> Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm:273
> -	   String wireBlockSignature = [NSMethodSignature
signatureWithObjCTypes:replyInfo->blockSignature.utf8().data()]._typeString.UTF
8String;
> +	   String wireBlockSignature = [NSMethodSignature
signatureWithObjCTypes:replyInfo->blockSignature.utf8().data()]._typeString;

Ditto.

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:289
> -    // Append the file name.    
> -    path.append(prefix.utf8().data(), prefix.length());
> +    // Append the file name.
> +    auto prefixAsUTF8 = prefix.utf8();
> +    path.append(prefixAsUTF8.data(), prefixAsUTF8.length());

Looks like this fixes a bug where filenames with non-ASCII characters might get
truncated because we would use the length in characters rather than the UTF-8
length in bytes.

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:292
> +    handle.m_sandboxExtension =
SandboxExtensionImpl::create(FileSystem::fileSystemRepresentation(String::fromU
TF8(path.data())).data(), type);

Any change here that the data is not valid UTF-8? If so we might have to check
the result of String::fromUTF8 for null; the Latin-1 constructor can never
fail, but UTF-8 decoding can.

>
Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.
mm:75
> +    clientIdentifier = String {
xpc_dictionary_get_string(m_initializerMessage, "client-identifier") };

Hoping these are all guaranteed to be ASCII or Latin-1.

> Source/WebKit/Shared/IPCTester.cpp:94
> +	   driverName = String { getenv("WEBKIT_MESSAGE_TEST_DEFAULT_DRIVER")
};

Encoding confusion here again, where we convert to String treating it as
Latin-1, then convert back to a C string below treating as UTF-8.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:285
> -    return resolvedPath;
> +    return String::fromUTF8(resolvedPath);

Good fix here, where we used Latin-1 before, not UTF-8.

> Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:235
> +    WebKit::NetworkCache::Data fileData = mapFile(path);

Seems like an auto would be good here.

> Source/WebKit/UIProcess/API/C/WKPage.cpp:518
> +    static API::String& sessionHistoryURLValueType =
API::String::create("SessionHistoryURL"_s).leakRef();

I would use auto& here.

> Source/WebKit/UIProcess/API/C/WKPage.cpp:524
> +    static API::String& sessionBackForwardListValueType =
API::String::create("SessionBackForwardListItem"_s).leakRef();

Ditto.

> Source/WebKit/UIProcess/API/Cocoa/APIContentRuleListStoreCocoa.mm:61
> +    return WTF::String::fromUTF8([contentRuleListStoreURL.get()
absoluteURL].path.fileSystemRepresentation);

Seems like we should just return path and let the NSString be converted to an
NSString, rather than calling fileSystemRepresentation and then
String::fromUTF8.

> Source/WebKit/UIProcess/API/Cocoa/WKContentRuleListStore.mm:82
> +    return
wrapper(API::ContentRuleListStore::storeWithPath(String::fromUTF8(url.absoluteU
RL.fileSystemRepresentation)));

Ditto, absoluteURL is an NSString.

> Source/WebKit/UIProcess/API/Cocoa/WKContentRuleListStore.mm:197
> +    return
wrapper(API::ContentRuleListStore::storeWithPath(String::fromUTF8(url.absoluteU
RL.fileSystemRepresentation)));

Ditto.

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:68
> +	  
launchOptions.extraInitializationData.add<HashTranslatorASCIILiteral>("user-dir
ectory-suffix"_s, String::fromUTF8(userDirectorySuffix));

Seems like the "fromUTF8" here could fail. In that case we probably want to
ignore it rather than adding a null?

> Source/WebKit/UIProcess/Cocoa/WebKitSwiftSoftLink.mm:44
> +	       auto webkitFrameworkDirectory =
WTF::FileSystemImpl::parentPath(String::fromUTF8(info.dli_fname));

Ditto.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:285
> +    m_resolvedPaths.uiProcessBundleResourcePath =
resolvePathForSandboxExtension(String { [[NSBundle mainBundle] resourcePath]
});

Why is this needed? Since resourcePath is an NSString, we should be able to
convert without an explicit call to the String constructor.

>
Source/WebKit/UIProcess/WebAuthentication/Virtual/VirtualHidConnection.cpp:225
> +	       auto attObj = buildAttestationMap(WTFMove(authenticatorData),
String { emptyString() }, { }, AttestationConveyancePreference::None);

I think we can just use the braces here without writing String. It does seem
peculiar if this takes a String&&, but I assume that is the case since we can’t
pass the const String&.

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:277
> +	   return WebKit::stringByResolvingSymlinksInPath(String {
cachePath.stringByStandardizingPath });

Why is this needed? Since stringByStandardizingPath returns an NSString, we
should be able to convert without an explicit call to the String constructor.

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:403
> +    return String::fromUTF8(url.absoluteURL.path.fileSystemRepresentation);

Should just return path, and let the NSString be converted to a String without
calling fileSystemRepresentation and then String::fromUTF8.

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:431
> +    return String::fromUTF8(url.absoluteURL.path.fileSystemRepresentation);

Ditto.

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:460
> +    return String::fromUTF8(url.absoluteURL.path.fileSystemRepresentation);

Ditto.

> Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:818
> -    RetainPtr<CABasicAnimation> animation = [CABasicAnimation
animationWithKeyPath:keyPath];
> +    auto nsKeyPath = adoptNS([[NSString alloc]
initWithBytes:keyPath.characters() length:keyPath.length()
encoding:NSISOLatin1StringEncoding]);
> +    RetainPtr<CABasicAnimation> animation = [CABasicAnimation
animationWithKeyPath:nsKeyPath.get()];

Seems like the helper to create an NSString from an ASCIILiteral should go
somewhere re-usable. Then we would not have to split this one line into two.
Even if it was just local to this file.

> Source/WebKit/webpushd/PushClientConnection.mm:155
> +   
sendDaemonMessage<DaemonMessageType::DebugMessage>(message.toStringWithoutCopyi
ng());

Seems suspect. If sendDaemonMessage really doesn’t need to hold onto the
string, which is required to make toStringWithoutCopying safe, then maybe it
should be changed to take StringView to make that explicit.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm:653
> +    _destinationPath = FileSystem::openTemporaryFile(String { filename },
fileHandle);

Why is this needed? I’d think that NSString to String is not explicit.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBFileName.mm:88
> +    NSString *existingDatbaseHash =
WebCore::SQLiteFileSystem::computeHashForFileName(String { existingDatabaseName
});
> +    NSString *createdDatabaseHash =
WebCore::SQLiteFileSystem::computeHashForFileName(String { createdDatabaseName
});
> +    NSString *unusedDatabaseHash =
WebCore::SQLiteFileSystem::computeHashForFileName(String { createdDatabaseName
});

Ditto.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBFileName.mm:131
> +    NSString *existingDatbaseHash =
WebCore::SQLiteFileSystem::computeHashForFileName(String { existingDatabaseName
});
> +    NSString *createdDatabaseHash =
WebCore::SQLiteFileSystem::computeHashForFileName(String { createdDatabaseName
});

Ditto.