[webkit-reviews] review granted: [Bug 230101] Add stub for File System Access API : [Attachment 437795] 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 230101: Add stub for File System Access API
https://bugs.webkit.org/show_bug.cgi?id=230101

Attachment 437795: Patch

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




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

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

> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.h:56
> +    FileSystemDirectoryHandle(String&&);

explicit.

> Source/WebCore/Modules/filesystemaccess/FileSystemFileHandle.h:37
> +    struct CreateWritableOptions {

Let's add it when needed.

> Source/WebCore/Modules/filesystemaccess/FileSystemFileHandle.h:45
> +    FileSystemFileHandle(String&&);

explicit.

> Source/WebCore/Modules/filesystemaccess/FileSystemHandle.h:49
> +    FileSystemHandle(Kind, String&& name);

Since it is refcounted, it is better to move the constructor to protected.

> Source/WebCore/Modules/filesystemaccess/FileSystemHandle.idl:32
> +    EnabledBySetting=FileSystemAccess

I am not sure this has any effect on dictionaries.

> Source/WebCore/Modules/filesystemaccess/FileSystemHandle.idl:47
> +    Promise<PermissionState> requestPermission(optional
FileSystemHandlePermissionDescriptor descriptor);

Are we sure we will quickly implement these two?
If not, it might be better to not include them so that feature detection works
by just checking that the method is defined.
Same questions for other methods.

> Source/WebCore/Modules/filesystemaccess/FileSystemPermissionDescriptor.idl:28
> +] dictionary FileSystemPermissionDescriptor : PermissionDescriptor {

Let's add this one and related when actually needed, I do not think this is
exposed to JS.
Ditto for struct FileSystemPermissionDescriptor

> Source/WebCore/Modules/filesystemaccess/FileSystemPermissionMode.h:30
> +enum class FileSystemPermissionMode : uint8_t {

could be bool I guess