Bug 238525 – Prepare WebKit/ & WebKitLegacy/ for making the String(const char*) constructor explicit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Prepare WebKit/, WebKitLegacy/ and Tools/ for making the String(const char*) constructor explicit.

[Chris Dumez]

Created attachment 456072 [details]
Patch

[Chris Dumez]

Created attachment 456079 [details]
Patch

[Chris Dumez]

Created attachment 456125 [details]
Patch

[Darin Adler]

Comment on attachment 456125 [details]
Patch

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

> Source/WTF/wtf/cocoa/VectorCocoa.h:54
> +inline RetainPtr<id> makeNSArrayElement(const char* string)
> +{
> +    return bridge_cast(adoptCF(CFStringCreateWithCString(nullptr, string, kCFStringEncodingISOLatin1)));
> +}

I hope this doesn't lead to encoding mistakes. If these are likely to be string literals, this could be UTF-8, not Latin-1, since that's what we use for most of our source code, although it’s probably against our rules to use them in string literals. Or could pass kCFStringEncodingASCII, which is not really better than Latin-1, I guess.

We could overload for ASCIILiteral instead or in addition if that was useful. Annoying maybe to have to use _s everywhere, but at least makes it less likely we’ll misinterpret a const char* that happens to be Latin-1 or UTF-8 as the other.

I guess leaving it Latin-1 makes it the same semantics as before since that’s what we do with const char* when constructing a String, but that’s really not good. I almost wish there was no constructor for String that took a const char* without being explicit about the character encoding. We could have fromASCII, fromLatin1, fromUTF8 some day. Well, making the constructor explicit is a step in the right direction.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2536
> +    if (!strcmp(tableName.characters(), "StorageAccessUnderTopFrameDomains"))

Seems like we should make operator==(ASCIILiteral, const char*) overload instead of writing strcmp on each line. Could be local to this file if you like. Of course, there might be slight ambiguity about whether it is pointer equality or string equality. Maybe it should be operator==(ASCIILiteral, ASCIILiteral) and we could add _s to all of these?

[Chris Dumez]

(In reply to Darin Adler from comment #4)
> Comment on attachment 456125 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456125&action=review
> 
> > Source/WTF/wtf/cocoa/VectorCocoa.h:54
> > +inline RetainPtr<id> makeNSArrayElement(const char* string)
> > +{
> > +    return bridge_cast(adoptCF(CFStringCreateWithCString(nullptr, string, kCFStringEncodingISOLatin1)));
> > +}
> 
> I hope this doesn't lead to encoding mistakes. If these are likely to be
> string literals, this could be UTF-8, not Latin-1, since that's what we use
> for most of our source code, although it’s probably against our rules to use
> them in string literals. Or could pass kCFStringEncodingASCII, which is not
> really better than Latin-1, I guess.
> 
> We could overload for ASCIILiteral instead or in addition if that was
> useful. Annoying maybe to have to use _s everywhere, but at least makes it
> less likely we’ll misinterpret a const char* that happens to be Latin-1 or
> UTF-8 as the other.
> 
> I guess leaving it Latin-1 makes it the same semantics as before since
> that’s what we do with const char* when constructing a String, but that’s
> really not good. I almost wish there was no constructor for String that took
> a const char* without being explicit about the character encoding. We could
> have fromASCII, fromLatin1, fromUTF8 some day. Well, making the constructor
> explicit is a step in the right direction.

I tried to maintain the previous behavior, which is why I used latin1. That said, I don't mind switching to UTF-8 for extra safety.
If I remember correctly, I needed this because our MIMETypeRegistry has several functions that return a FixedVector<const char*> and those get converted to NSArrays. It used to do an implicit conversion of each item to WTF::String.

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2536
> > +    if (!strcmp(tableName.characters(), "StorageAccessUnderTopFrameDomains"))
> 
> Seems like we should make operator==(ASCIILiteral, const char*) overload
> instead of writing strcmp on each line. Could be local to this file if you
> like. Of course, there might be slight ambiguity about whether it is pointer
> equality or string equality. Maybe it should be operator==(ASCIILiteral,
> ASCIILiteral) and we could add _s to all of these?

Good idea.

[Darin Adler]

(In reply to Chris Dumez from comment #5)
> If I remember correctly, I needed this because our MIMETypeRegistry has
> several functions that return a FixedVector<const char*> and those get
> converted to NSArrays.

Maybe those should be changed to FixedVector<ASCIILiteral> instead.

[Darin Adler]

(In reply to Darin Adler from comment #6)
> Maybe those should be changed to FixedVector<ASCIILiteral> instead.

Or Span<const ASCIILiteral>.

[Chris Dumez]

(In reply to Darin Adler from comment #6)
> (In reply to Chris Dumez from comment #5)
> > If I remember correctly, I needed this because our MIMETypeRegistry has
> > several functions that return a FixedVector<const char*> and those get
> > converted to NSArrays.
> 
> Maybe those should be changed to FixedVector<ASCIILiteral> instead.

I had tried that at first and failed. That said, it is probably doable. I am just very unfamiliar with FixedVector / makeFixedVector and ComparableCaseFoldingASCIILiteral.

[Chris Dumez]

(In reply to Chris Dumez from comment #8)
> (In reply to Darin Adler from comment #6)
> > (In reply to Chris Dumez from comment #5)
> > > If I remember correctly, I needed this because our MIMETypeRegistry has
> > > several functions that return a FixedVector<const char*> and those get
> > > converted to NSArrays.
> > 
> > Maybe those should be changed to FixedVector<ASCIILiteral> instead.
> 
> I had tried that at first and failed. That said, it is probably doable. I am
> just very unfamiliar with FixedVector / makeFixedVector and
> ComparableCaseFoldingASCIILiteral.

I will give it another try.

[Darin Adler]

Might not be worth it. Not sure that ComparableCaseFoldingASCIILiteral is compatible with ASCIILiteral, despite the names.

[Chris Dumez]

Created attachment 456163 [details]
Patch

[Chris Dumez]

I added the operator==(ASCIILiteral, const char*) and operator==(ASCIILiteral, ASCIILiteral) operators as suggested by Darin.

I also dropped the changes to VectorCocoa.h and updated our MIMETypeRegistry functions to return FixedVector<ASCIILiteral> instead of FixedVector<const char*>.

[Chris Dumez]

Comment on attachment 456163 [details]
Patch

And I broke the iOS build..

[Chris Dumez]

Created attachment 456165 [details]
Patch

[Chris Dumez]

Created attachment 456179 [details]
Patch

[Darin Adler]

Comment on attachment 456179 [details]
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/PrivateClickMeasurementDatabase.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.cpp.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.UTF8String;
> +        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::fromUTF8(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.absoluteURL.fileSystemRepresentation)));

Ditto, absoluteURL is an NSString.

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

Ditto.

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:68
> +        launchOptions.extraInitializationData.add<HashTranslatorASCIILiteral>("user-directory-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.toStringWithoutCopying());

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.

[Chris Dumez]

(In reply to Darin Adler from comment #16)
> Comment on attachment 456179 [details]
> 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?

I mentioned this during one of your earlier reviews but StringView does have a constructor that takes in an ASCIILiteral. However, this constructor is marked as explicit for some reason.
This is why I am using "Record" (I feel `StringView { "Record"_s }` would be too verbose).

As I said last time, I don't know that there is a good reason for that constructor to be marked as explicit though. As far as I can tell, constructing a StringView from an ASCIILiteral is both always safe and efficient?
What do you think about dropping the explicit so that I can use _s more consistently?

[Darin Adler]

(In reply to Chris Dumez from comment #17)
> What do you think about dropping the explicit so that I can use _s more
> consistently?

I like the idea. I think that we should try it and see if it causes any problems.

[Chris Dumez]

Comment on attachment 456179 [details]
Patch

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

>> Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDatabase.cpp:716
>> +    constexpr ASCIILiteral attributedTableName = "AttributedPrivateClickMeasurement"_s;
> 
> Why not auto here?

No good reason, will update.

>> Source/WebKit/Scripts/PreferencesTemplates/WebPreferencesExperimentalFeatures.cpp.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?

The issue is that we don't have an ASCIILiteral, we have things that are defined in the yaml like "", "foo" or { }, or defaultFooValue().

I could modify the template here to append the `_s` suffix to @pref.humanReadableName and and @pref.humanReadableDescription. I am not particularly good with Ruby and it seems pretty awkward to do inside the ERB? That why I didn't bother.
I tried using ASCIILiterals in the yaml also but that leads to other issues because we have some code trying to Objc "box" (e.g `@(PSON_DEFAULT_VALUE)`) those things.

For WebCore settings, I did update the erb to append `_s` in the ERB when needed but it was a little simpler (there was only one value in the line that I had to worry about, not 2). Here, I decided not to bother but we can revisit.

>> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:292
>> +    handle.m_sandboxExtension = SandboxExtensionImpl::create(FileSystem::fileSystemRepresentation(String::fromUTF8(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.

That's a good point. I'll add a null check to be safe.

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

Yes, I wasn't 100% sure about this one. I maintained existing behavior at least but I guess we could fromUTF8() to be safe.

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

I wasn't sure if this variable could contain UTF8. Will switch to fromUTF8() to be safe.

>> Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:235
>> +    WebKit::NetworkCache::Data fileData = mapFile(path);
> 
> Seems like an auto would be good here.

Indeed, will fix.

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

OK.

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

Ok.

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

Ok.

>> Source/WebKit/UIProcess/API/Cocoa/WKContentRuleListStore.mm:82
>> +    return wrapper(API::ContentRuleListStore::storeWithPath(String::fromUTF8(url.absoluteURL.fileSystemRepresentation)));
> 
> Ditto, absoluteURL is an NSString.

Ok.

>> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:68
>> +        launchOptions.extraInitializationData.add<HashTranslatorASCIILiteral>("user-directory-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?

Ok.

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

Ok.

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

Because the function wants a StringView. I cannot implicitly construct a StringView from a NSString. Note that an implicit conversion to String was already happening. It is just explicit now since I updated resolvePathForSandboxExtension() to take a StringView instead of a String.

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

Yes, this takes a String&&, which was annoying. I'll try the curly brackets.

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

Same as above the function wants a StringView, not a String (or NSString).

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

Ok.

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

Ok.

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

Ok.

>> Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:818
>> +    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.

Ok, I will look into that.

>> Source/WebKit/webpushd/PushClientConnection.mm:155
>> +    sendDaemonMessage<DaemonMessageType::DebugMessage>(message.toStringWithoutCopying());
> 
> 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.

It really doesn't hold the String, it merely encodes it to do XPC:
```
void ClientConnection::sendDaemonMessage(Args&&... args) const
{
    if (!m_xpcConnection)
        return;

    Encoder encoder;
    encoder.encode(std::forward<Args>(args)...);
```

Not sure our Encoder would like a StringView but I can look into it. That said, if it is too annoying, I might just to toString() for safety (I don't think we're too worried about performance here).

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

Because openTemporaryFile() takes a StringView.

[Darin Adler]

Comment on attachment 456179 [details]
Patch

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

>>> Source/WebKit/webpushd/PushClientConnection.mm:155
>>> +    sendDaemonMessage<DaemonMessageType::DebugMessage>(message.toStringWithoutCopying());
>> 
>> 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.
> 
> It really doesn't hold the String, it merely encodes it to do XPC:
> ```
> void ClientConnection::sendDaemonMessage(Args&&... args) const
> {
>     if (!m_xpcConnection)
>         return;
> 
>     Encoder encoder;
>     encoder.encode(std::forward<Args>(args)...);
> ```
> 
> Not sure our Encoder would like a StringView but I can look into it. That said, if it is too annoying, I might just to toString() for safety (I don't think we're too worried about performance here).

I think it’s good for our Encoder to take StringView if it’s straightforward to allow that. I agree that we shouldn’t work too hard to fix this, but if it’s easy enough it would be nice to keep this call site simpler.

[Darin Adler]

Comment on attachment 456179 [details]
Patch

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

>>> 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.
> 
> Because the function wants a StringView. I cannot implicitly construct a StringView from a NSString. Note that an implicit conversion to String was already happening. It is just explicit now since I updated resolvePathForSandboxExtension() to take a StringView instead of a String.

If there is any performance-critical code that needs to pass a CFStringRef or NSString to a StringView, we can make a version that avoids memory allocation using a technique like StringView::UpconvertedCharacters. We can make a data structure that holds a fixed number of characters that is almost always enough to hold the string, either LChar or UChar based on whether we can efficiently decide if we can use the LChar, and then do heap allocation only if there are more characters. Could be as simple as Vector<UChar, xxx>. Then we won’t do any heap allocation for the common cases.

We can make this syntactically just as simple as converting to a String. If performance is never an issue, and we just want to avoid writing more code, then we can keep using String, it just means we’ll always allocate on the heap and also needlessly deal with the reference count.

[Darin Adler]

Only real risk of the UpconvertedCharacters-style approach is that if we use it multiple times in the same function we might end up making a function that needlessly uses a lot of stack space.

[Chris Dumez]

Comment on attachment 456179 [details]
Patch

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

>>> 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&.
> 
> Yes, this takes a String&&, which was annoying. I'll try the curly brackets.

FYI, your proposal didn't build:
```
/Volumes/Data/WebKit/OpenSource/Source/WebKit/UIProcess/WebAuthentication/Virtual/VirtualHidConnection.cpp:225:27: error: no matching function for call to 'buildAttestationMap'
            auto attObj = buildAttestationMap(WTFMove(authenticatorData), { emptyString() }, { }, AttestationConveyancePreference::None);
In file included from /Volumes/Data/WebKit/OpenSource/Source/WebKit/UIProcess/WebAuthentication/Virtual/VirtualHidConnection.cpp:36:
/Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/WebCore.framework/PrivateHeaders/WebAuthenticationUtils.h:52:42: note: candidate function not viable: 2nd argument ('const WTF::String') would lose const qualifier
WEBCORE_EXPORT cbor::CBORValue::MapValue buildAttestationMap(Vector<uint8_t>&&, String&&, cbor::CBORValue::MapValue&&, const AttestationConveyancePreference&);
1 error generated.
```

[Chris Dumez]

Created attachment 456241 [details]
Patch

[Chris Dumez]

Created attachment 456244 [details]
Patch

[Chris Dumez]

Created attachment 456305 [details]
Patch

[Chris Dumez]

Comment on attachment 456305 [details]
Patch

Clearing flags on attachment: 456305

Committed r292197 (249100@trunk): <https://commits.webkit.org/249100@trunk>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/91141513>