[webkit-reviews] review granted: [Bug 234520] Ensure file handles used in FileSystemAccess API are closed : [Attachment 447619] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 234520: Ensure file handles used in FileSystemAccess API are closed
https://bugs.webkit.org/show_bug.cgi?id=234520

Attachment 447619: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 447619
  --> https://bugs.webkit.org/attachment.cgi?id=447619
Patch

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

> Source/WTF/wtf/CrossThreadCopier.h:182
> +    static Type copy(Type&& source)
> +    {
> +	   return
std::make_pair(CrossThreadCopier<F>::copy(WTFMove(source.first)),
CrossThreadCopier<S>::copy(WTFMove(source.second)));
> +    }

Do we really want to call a cross-thread move operation "copy"? Maybe there’s
no way to avoid it, but it seems wrong.

> Source/WebCore/dom/ExceptionOr.h:222
> +    static Type copy(Type&& source)

Same thought. This is a move, not a a copy, I think.

> Source/WebCore/platform/FileHandle.h:37
> +    WTF_MAKE_NONCOPYABLE(FileHandle);

I am not sure it’s better to use this macro than deleting the copy and
assignment operators. We invented this macro back when there was no simple way
to write this in a class. I kind of wish we didn’t use a macro for this, since
it’s now a pretty simple idiom.

> Source/WebCore/platform/FileHandle.h:39
> +    static FileHandle adopt(FileSystem::PlatformFileHandle&);

This should take an rvalue reference, not an lvalue reference. That way we can
pass it a file handle returned by a function, and don’t require using a local
variable for it.

> Source/WebCore/platform/FileHandle.h:56
> +    FileHandle isolatedCopy() &&;

This operation is a move, not a copy, I think. Not sure it should be named
isolatedCopy.

> Source/WebCore/platform/FileHandle.h:59
> +    FileHandle(FileSystem::PlatformFileHandle);

This function should take an rvalue reference, and I think should do the same
thing as adopt. In fact, given this can take an rvalue reference, I think we
can do away with the named adopt function entirely. You’ll be able to see the
adoption because the caller will use WTFMove or std::exchange.

> Source/WebKit/Platform/IPC/SharedFileHandle.h:39
> +    static std::optional<SharedFileHandle>
create(FileSystem::PlatformFileHandle&);

This should take an rvalue reference, not an lvalue reference. That way we can
pass it a file handle returned by a function, and don’t require using a local
variable for it.

> Source/WebKit/Platform/IPC/SharedFileHandle.h:48
> +    explicit SharedFileHandle(FileSystem::PlatformFileHandle& handle)

This should take an rvalue reference, not an lvalue reference.

> Source/WebKit/Platform/IPC/cocoa/SharedFileHandleCocoa.cpp:34
> +std::optional<SharedFileHandle>
SharedFileHandle::create(FileSystem::PlatformFileHandle& handle)

This should take an rvalue reference, not an lvalue reference.

>
Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.cpp:152
> +	   auto resultValue = WTFMove(result.value());

Why do we have to move this into a local variable? Can’t we just use a
reference to it in place rather than moving it?