Bug 236611 – Migrate IndexedDB and LocalStorage data to GeneralStorageDirectory

• Component: Website Storage
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

Created attachment 451947 [details]
Patch

[Chris Dumez]

Comment on attachment 451947 [details]
Patch

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

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:181
>          auto files = FileSystem::listDirectory(m_rootPath);

Can we please add an assertion that this is called off the main thread given that it does file system operations?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:193
> +        FileSystem::deleteEmptyDirectory(currentIDBStoragePath);

I will say it is fishy that a function called "isEmpty()" tries to remove a directory on disk (even an empty one).

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:247
> +        if (!m_resolvedIDBStoragePath.isNull())

Can we please add an assertion that this is called off the main thread given that it does file system operations?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:251
> +            ASSERT(!(m_customIDBStoragePath.isEmpty() ^ m_rootPath.isEmpty()));

Odd to be using ^ here. Would this be identical:
`ASSERT(m_customIDBStoragePath.isEmpty() == m_rootPath.isEmpty()));` ?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:359
> +            ASSERT(!(m_customLocalStoragePath.isEmpty() ^ m_rootPath.isEmpty()));

Ditto. Not very readable.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:79
> +    bool m_useCustomPaths;

Can we move all the booleans together and at the end for better packing?

> Source/WebKit/Shared/WebsiteDataStoreParameters.cpp:165
> +    parameters.useCustomStoragePaths = WTFMove(*useCustomStoragePaths);

No WTFMove() for a boolean.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreConfiguration.h:135
> +    bool useCustomStoragePaths() const { return m_useCustomStoragePaths; }

This is not a good name for a getter, this is why we usually have prefixes for getters in Webkit.
For example, I think we would usually name this `shouldUseCustomStoragePaths`. Alternatively, at the WebKit API layer, we also use things like "usesCustomStoragePaths()". "useCustomStoragePage()" for a getter is confusing though.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreConfiguration.h:210
> +    bool m_useCustomStoragePaths { true };

Ditto about naming.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:851
> +        configuration = nil;

These 3 commands should not be needed since they are scope local and we're about to go out of scope.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:934
> +        configuration = nil;

ditto.

[Sihui Liu]

Comment on attachment 451947 [details]
Patch

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

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:181
>>          auto files = FileSystem::listDirectory(m_rootPath);
> 
> Can we please add an assertion that this is called off the main thread given that it does file system operations?

Sure.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:193
>> +        FileSystem::deleteEmptyDirectory(currentIDBStoragePath);
> 
> I will say it is fishy that a function called "isEmpty()" tries to remove a directory on disk (even an empty one).

I will change it to check if the idb directory has any files in it.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:247
>> +        if (!m_resolvedIDBStoragePath.isNull())
> 
> Can we please add an assertion that this is called off the main thread given that it does file system operations?

Sure.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:251
>> +            ASSERT(!(m_customIDBStoragePath.isEmpty() ^ m_rootPath.isEmpty()));
> 
> Odd to be using ^ here. Would this be identical:
> `ASSERT(m_customIDBStoragePath.isEmpty() == m_rootPath.isEmpty()));` ?

Sure.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:359
>> +            ASSERT(!(m_customLocalStoragePath.isEmpty() ^ m_rootPath.isEmpty()));
> 
> Ditto. Not very readable.

Will change.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:79
>> +    bool m_useCustomPaths;
> 
> Can we move all the booleans together and at the end for better packing?

Okay.

>> Source/WebKit/Shared/WebsiteDataStoreParameters.cpp:165
>> +    parameters.useCustomStoragePaths = WTFMove(*useCustomStoragePaths);
> 
> No WTFMove() for a boolean.

Will remove.

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreConfiguration.h:135
>> +    bool useCustomStoragePaths() const { return m_useCustomStoragePaths; }
> 
> This is not a good name for a getter, this is why we usually have prefixes for getters in Webkit.
> For example, I think we would usually name this `shouldUseCustomStoragePaths`. Alternatively, at the WebKit API layer, we also use things like "usesCustomStoragePaths()". "useCustomStoragePage()" for a getter is confusing though.

Will change to shouldUseCustomStoragePaths.

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreConfiguration.h:210
>> +    bool m_useCustomStoragePaths { true };
> 
> Ditto about naming.

Will change.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:851
>> +        configuration = nil;
> 
> These 3 commands should not be needed since they are scope local and we're about to go out of scope.

Sure.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:934
>> +        configuration = nil;
> 
> ditto.

Sure.

[Sihui Liu]

Created attachment 452130 [details]
Patch for landing

[Sihui Liu]

Created attachment 452131 [details]
Patch for landing

[EWS]

Committed r289878 (247316@main): <https://commits.webkit.org/247316@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/89008688>