Bug 236727 – Ensure NetworkStorageManager::moveData performs move operation based on data types

• Component: Website Storage
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

rdar://89009881

[Sihui Liu]

Created attachment 452229 [details]
Patch

[Chris Dumez]

Comment on attachment 452229 [details]
Patch

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

r=me

> Source/WebKit/ChangeLog:3
> +        Ensure NetworkStorageManager::moveData perform move operation based on data types

perform -> performs

> Source/WebKit/ChangeLog:9
> +        In r286936, we introduced NetworkStorageManager::moveData to move data from one origin from another origin.

"from one origin from another origin" doesn't really make sense. Maybe you mean "from one origin to another".

> Source/WebKit/ChangeLog:11
> +        NetworkStorageManager would try to move every storage type it manages. This has caused unexpected failure, as

"failures" or "an unexpected failure"

> Source/WebKit/ChangeLog:16
> +        Updated existing API test to ensure only expected type of data is moved.

"type of data is moved" -> "data types are moved"

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:221
> +    void moveData(OptionSet<WebsiteDataType> types, const String& localStoragePath, const String& idbStoragePath)

Would be nice to move the implementation out of the class to improve readability. This is too huge to be inline IMO.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:223
> +        // This is only supported for IndexedDB and LocalStorage now.

Do we need to add an assertion to make sure we only get types we can handle?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:247
> +    String resolvedLocalStoragePath()

Would be nice to move the implementation out of the class to improve readability. This is too huge to be inline IMO.

[Sihui Liu]

Comment on attachment 452229 [details]
Patch

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

>> Source/WebKit/ChangeLog:3
>> +        Ensure NetworkStorageManager::moveData perform move operation based on data types
> 
> perform -> performs

Updated.

>> Source/WebKit/ChangeLog:9
>> +        In r286936, we introduced NetworkStorageManager::moveData to move data from one origin from another origin.
> 
> "from one origin from another origin" doesn't really make sense. Maybe you mean "from one origin to another".

Yes it should be "to".

>> Source/WebKit/ChangeLog:16
>> +        Updated existing API test to ensure only expected type of data is moved.
> 
> "type of data is moved" -> "data types are moved"

Okay.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:221
>> +    void moveData(OptionSet<WebsiteDataType> types, const String& localStoragePath, const String& idbStoragePath)
> 
> Would be nice to move the implementation out of the class to improve readability. This is too huge to be inline IMO.

We may move all the StorageBucket functions outside of the class altogether. I will do it in a followup.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:223
>> +        // This is only supported for IndexedDB and LocalStorage now.
> 
> Do we need to add an assertion to make sure we only get types we can handle?

There's an assertion in:
- (void)_renameOrigin:(NSURL *)oldName to:(NSURL *)newName forDataOfTypes:(NSSet<NSString *> *)dataTypes completionHandler:(void (^)(void))completionHandler

[Sihui Liu]

Created attachment 452271 [details]
Patch

[EWS]

Committed r290033 (247413@main): <https://commits.webkit.org/247413@main>

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