[webkit-reviews] review granted: [Bug 236611] Migrate IndexedDB and LocalStorage data to GeneralStorageDirectory : [Attachment 451947] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 236611: Migrate IndexedDB and LocalStorage data to GeneralStorageDirectory
https://bugs.webkit.org/show_bug.cgi?id=236611

Attachment 451947: Patch

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




--- Comment #2 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 451947
  --> https://bugs.webkit.org/attachment.cgi?id=451947
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.