[webkit-reviews] review granted: [Bug 233567] Fetch and remove file system data via WKWebsiteDataStore : [Attachment 445813] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 233567: Fetch and remove file system data via WKWebsiteDataStore
https://bugs.webkit.org/show_bug.cgi?id=233567

Attachment 445813: Patch

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




--- Comment #16 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 445813
  --> https://bugs.webkit.org/attachment.cgi?id=445813
Patch

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

> Source/WTF/wtf/FileSystem.cpp:517
> +std::optional<Vector<uint8_t>> readEntireFile(PlatformFileHandle handle)

I can see a few places in our code base that could make use of it
(createForFile, Engine::readSizeFile).
Let's file a bug to make use of this routine across the code base.

> Source/WebCore/Modules/filesystemaccess/FileSystemStorageConnection.h:30
> +#include "ProcessQualified.h"

Is that needed? Should it be in a cpp file instead?

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:49
> +    auto originFileHandle = FileSystem::openFile(filePath,
FileSystem::FileOpenMode::ReadWrite);

Should it be FileSystem::FileOpenMode::Read?

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:389
> +void NetworkStorageManager::forEachOriginDirectory(Function<void(const
String& directory)>&& apply)

apply can probably be const&?
No need for directory name?

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:456
> +	       // If origin cannot be retrieved, but we are asked to remove
data for all origins, remove it.

If origin cannot be retrieved, we probably lost all this data and will not be
able to reuse it in the future.
We should probably add release logging to catch this case.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:104
> +	       m_fileSystemStorageManager = nullptr;

Why not m_fileSystemStorageManager = nullptr; unconditionally?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:156
> +    defaultBucket().deleteData(types, modifiedSince);

ASSERT(!RunLoop::isMain());?