Bug 214905 – -[WKWebsiteDataStore _renameOrigin:] needs to support IndexedDB renames

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Timothy Hatcher]

For Web Extensions in Safari, we need to rename origins between launches. Right now we are using the -[WKWebsiteDataStore _renameOrigin:] SPI, but it only supports LocalStorage. We need to support IndexedDB as well.

See NetworkProcess::renameOriginInWebsiteData() for where the additional WebsiteDataType::IndexedDBDatabases type support needs added.

[Radar WebKit Bug Importer]

<rdar://problem/66247978>

[Radar WebKit Bug Importer]

<rdar://problem/66248009>

[Timothy Hatcher]

<rdar://problem/66247978>

[Timothy Hatcher]

<rdar://problem/64790993>

[Sihui Liu]

Created attachment 405614 [details]
Patch

[Sihui Liu]

Created attachment 405615 [details]
Patch

[Timothy Hatcher]

Comment on attachment 405615 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:616
> +    auto filteredTypes = [dataTypes objectsPassingTest:^BOOL(id obj, BOOL *stop) {
> +        NSString *type = (NSString *)obj;
> +        NSSet *supportedTypes = [NSSet setWithObjects:WKWebsiteDataTypeLocalStorage, WKWebsiteDataTypeIndexedDBDatabases, nil];
> +        return ![supportedTypes containsObject:type];
> +    }];
> +    if (filteredTypes.count) {

A better way to do this would be:

NSSet *supportedTypes = [NSSet setWithObjects:WKWebsiteDataTypeLocalStorage, WKWebsiteDataTypeIndexedDBDatabases, nil];
if (![dataTypes isSubsetOfSet:supportedTypes])
    [NSException raise:...];

Calling the completion handler after the exception is not needed. The exception ended execution of the method.

[Sihui Liu]

Comment on attachment 405615 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:616
>> +    if (filteredTypes.count) {
> 
> A better way to do this would be:
> 
> NSSet *supportedTypes = [NSSet setWithObjects:WKWebsiteDataTypeLocalStorage, WKWebsiteDataTypeIndexedDBDatabases, nil];
> if (![dataTypes isSubsetOfSet:supportedTypes])
>     [NSException raise:...];
> 
> Calling the completion handler after the exception is not needed. The exception ended execution of the method.

Will update!

[Sihui Liu]

Created attachment 405623 [details]
Patch

[Alex Christensen]

Comment on attachment 405623 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:721
> +    renameOriginForVersion("v0");

Why would we want to rename v0?  The SPI user of this will only have v1, I think.

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:101
> +        postTaskReply(CrossThreadTask([callback = WTFMove(callback)]() mutable {

Could this be simpler instead of making a lambda just to wrap a CompletionHandler?
postTaskReply(CrossThreadTask(WTFMove(callback)));

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:561
> +    RetainPtr<WebsiteDataStoreCustomPathsMessageHandler> handler = adoptNS([[WebsiteDataStoreCustomPathsMessageHandler alloc] init]);

These could use auto.

[Sihui Liu]

(In reply to Alex Christensen from comment #10)
> Comment on attachment 405623 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405623&action=review
> 
> > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:721
> > +    renameOriginForVersion("v0");
> 
> Why would we want to rename v0?  The SPI user of this will only have v1, I
> think.
> 

I have no idea about that so I rename both. Will update.

> > Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:101
> > +        postTaskReply(CrossThreadTask([callback = WTFMove(callback)]() mutable {
> 
> Could this be simpler instead of making a lambda just to wrap a
> CompletionHandler?
> postTaskReply(CrossThreadTask(WTFMove(callback)));
> 
Yes.

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:561
> > +    RetainPtr<WebsiteDataStoreCustomPathsMessageHandler> handler = adoptNS([[WebsiteDataStoreCustomPathsMessageHandler alloc] init]);
> 
> These could use auto.

Okay.

[Sihui Liu]

Created attachment 405702 [details]
Patch for landing

[EWS]

Committed r265149: <https://trac.webkit.org/changeset/265149>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405702 [details].