[webkit-reviews] review granted: [Bug 232605] FileSystemSyncAccessHandle should be invalidated when network process crashes : [Attachment 445529] 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 232605: FileSystemSyncAccessHandle should be invalidated when network
process crashes
https://bugs.webkit.org/show_bug.cgi?id=232605

Attachment 445529: Patch

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




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

WinCairo issue is legit, probably a missing include.

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

> Source/WebCore/Modules/filesystemaccess/FileSystemFileHandle.cpp:100
> +void
FileSystemFileHandle::registerSyncAccessHandle(FileSystemSyncAccessHandleIdenti
fier identifier, FileSystemSyncAccessHandle* handle)

FileSystemSyncAccessHandle& would be better.

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:52
> +    m_source->registerSyncAccessHandle(m_identifier, this);

*this would be better.

>
Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:3
19
> +	   handle->invalidate();

if (auto handle...)

>
Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.h:73
> +    void registerSyncAccessHandle(FileSystemSyncAccessHandleIdentifier,
ScriptExecutionContextIdentifier, FileSystemSyncAccessHandle*) final;

FileSystemSyncAccessHandle& is better.
I would tend to make this registerSyncAccessHandle not virtual but specific to
WorkerFileSystemStorageConnection.
To call it, handle would need to do something like
downcast<WorkerGlobalScope>(context).fileSystemStorageConnection().registerSync
AccessHandle(...).
We could then take a reference instead of a pointer.

FileSystemStorageConnection could have a
registerSyncAccessHandle(FileSystemSyncAccessHandleIdentifier,
ScriptExecutionContextIdentifier) that would do nothing in
WorkerFileSystemStorageConnection.
Or we could somehow introduce a MainThreadFileSystemStorageConnection.

> Source/WebCore/workers/WorkerGlobalScope.h:96
> +    WEBCORE_EXPORT WorkerFileSystemStorageConnection*
fileSystemStorageConnection();

Can we return a reference here?

>
Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.cpp:215
> +	       if (FileSystemStorageConnection* connection =
downcast<WebCore::WorkerGlobalScope>(context).fileSystemStorageConnection())

auto*. Would be good to be able to use a ref.