[webkit-reviews] review granted: [Bug 230861] Replace FileSystemHandleImpl with FileSystemStorageConnection : [Attachment 439539] 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 230861: Replace FileSystemHandleImpl with FileSystemStorageConnection
https://bugs.webkit.org/show_bug.cgi?id=230861

Attachment 439539: Patch

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




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

Looks good to me. If possible, it would be good to see if we can simplify the
ID management to not do any conversion, either here or as a follow-up.

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

> Source/WebCore/Modules/storage/StorageConnection.h:44
> +    using GetDirectoryCallback =
CompletionHandler<void(ExceptionOr<FileSystemHandleIdentifier>&&,
RefPtr<FileSystemStorageConnection>&&)>;

I guess it could be an ExceptionOr<std::pair<FileSystemHandleIdentifier,
Ref<FileSystemStorageConnection>>>.
Or maybe the callsite could retrieve the connection somehow to just return the
id.

> Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.cpp:55
> +	   return completionHandler(WebCore::Exception { WebCore::UnknownError,
"Connection is lost" });

For now, I would do:
if (!m_connection)
    m_connection = WebProcess::singleton()....

> Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.cpp:64
> +    });

Can you pass the completionHandler directly?

> Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.cpp:81
> +	  
completionHandler(makeObjectIdentifier<WebCore::FileSystemHandleIdentifierType>
(storageIdentifier.toUInt64()));

Do we need two different types, FileSystemHandleIdentifier and
FileSystemStorageHandleIdentifier?
It would be convenient to just use one if we can.

> Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.h:49
> +    WebFileSystemStorageConnection(IPC::Connection&);

explicit

> Source/WebKit/WebProcess/WebProcess.cpp:1215
> +	   m_fileSystemStorageConnection = nullptr;

We might not need to set it to nullptr if we get a new IPC::Connection directly
in m_fileSystemStorageConnection