...
rdar://89009881
Created attachment 452229 [details] Patch
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.
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
Created attachment 452271 [details] Patch
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].