Bug 231185 – Add support for FileSystemSyncAccessHandle

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

...

[Sihui Liu]

Created attachment 440085 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/83847859>

[Sihui Liu]

Created attachment 440086 [details]
Patch

[youenn fablet]

Comment on attachment 440086 [details]
Patch

Looks good overall, some comments below.

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

> Source/WebCore/ChangeLog:9
> +        https://github.com/WICG/file-system-access/blob/main/AccessHandle.md

Different API so probably would requite an additional experimental feature flag.

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:42
> +    , m_identifier(identifier)

Typed identifier?
Given it seems like there will be only one sync handle at a time, network process could generate this identifier and give it a type.

> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.h:66
> +    uint64_t m_nextAccessHandleIdentifier { 1 };

Best as an ObjectIdentifier but otherwise we usually set it to 0 and pre increment it before using it.

> Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.cpp:156
> +            return completionHandler(WebCore::Exception { convertToExceptionCode(error.value()) });

Could write a static routine that would convert std::optional<Error> into ExceptionOr<void>, then you could just write completionHandle(myRoutine(WTFMove(result))) here and below VoidCallback.

[Sihui Liu]

Created attachment 440193 [details]
Patch

[Sihui Liu]

Created attachment 440224 [details]
Patch

[Sihui Liu]

Created attachment 440226 [details]
Patch

[Sihui Liu]

Created attachment 440230 [details]
Patch

[Sihui Liu]

Created attachment 440233 [details]
Patch

[youenn fablet]

Comment on attachment 440233 [details]
Patch

Looks almost ready, WPE bot is failing though.
Also, I think the idea is to keep the file opened, or that there is a buffer somewhere in network process, so that flush has a use.

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

> Source/WebCore/Modules/filesystemaccess/FileSystemFileHandle.idl:34
> +    [Exposed=Worker] Promise<FileSystemSyncAccessHandle> createSyncAccessHandle();

Should be runtime enabled, or moved to something like FileSystemFileHandle+SyncAccess.idl

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:51
> +        m_source->close(m_identifier, [](auto) { });

We could do that a bit earlier if it were a ContextDestructionObserver.
That might be better than to wait for GC.

> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:162
> +    if (!m_activeSyncAccessHandle || m_activeSyncAccessHandle.value() != accessHandleIdentifier)

I would tend to use the shorter *m_activeSyncAccessHandle syntax

> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:181
> +    auto closeFileScope = makeScopeExit([&] {

Shouldn't the FileSystemSyncAccessHandleIdentifier keep the file opened so that it gets faster?
And to be able to write to it progressively?

> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:205
> +    auto result = FileSystem::flushFile(handle);

Why doing flush on a just opened file?

> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.cpp:31
> +#include <wtf/Scope.h>

Needed?

> Source/WebKit/WebProcess/WebCoreSupport/WebStorageConnection.cpp:59
> +            return completionHandler(convertToExceptionOr(result.error()).releaseException());

I would be tempted to add two routines, one for Error and one for std::optional<Error>

[Sihui Liu]

(In reply to youenn fablet from comment #10)
> Comment on attachment 440233 [details]
> Patch
> 
> Looks almost ready, WPE bot is failing though.
> Also, I think the idea is to keep the file opened, or that there is a buffer
> somewhere in network process, so that flush has a use.

The WPE fix seems to be trivial so I thought I will just let the bots run the tests first :P.
Yes, the idea is to keep the file opened once when the handle created for reads and writes. Since read and write are not included in this patch, I try to make the patch self contained by not adding that (since why would we need to keep file open when we just need to get the size, etc.)

> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440233&action=review
> 
> > Source/WebCore/Modules/filesystemaccess/FileSystemFileHandle.idl:34
> > +    [Exposed=Worker] Promise<FileSystemSyncAccessHandle> createSyncAccessHandle();
> 
> Should be runtime enabled, or moved to something like
> FileSystemFileHandle+SyncAccess.idl

Sure.

> 
> > Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:51
> > +        m_source->close(m_identifier, [](auto) { });
> 
> We could do that a bit earlier if it were a ContextDestructionObserver.
> That might be better than to wait for GC.

I guess I can make it ContextDestructionObserver.

> 
> > Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:162
> > +    if (!m_activeSyncAccessHandle || m_activeSyncAccessHandle.value() != accessHandleIdentifier)
> 
> I would tend to use the shorter *m_activeSyncAccessHandle syntax

Okay.

> 
> > Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:181
> > +    auto closeFileScope = makeScopeExit([&] {
> 
> Shouldn't the FileSystemSyncAccessHandleIdentifier keep the file opened so
> that it gets faster?
> And to be able to write to it progressively?

Yes, it should. But since write is not included in the patch yet, I just close the file after operation.

> 
> > Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:205
> > +    auto result = FileSystem::flushFile(handle);
> 
> Why doing flush on a just opened file?

Good point, this should be a no-op for now.

> 
> > Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.cpp:31
> > +#include <wtf/Scope.h>
> 
> Needed?

ah will remove.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebStorageConnection.cpp:59
> > +            return completionHandler(convertToExceptionOr(result.error()).releaseException());
> 
> I would be tempted to add two routines, one for Error and one for
> std::optional<Error>

Okay.

[Sihui Liu]

Created attachment 440248 [details]
Patch

[Sihui Liu]

Created attachment 440252 [details]
Patch

[Sihui Liu]

Comment on attachment 440233 [details]
Patch

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

>>> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:51
>>> +        m_source->close(m_identifier, [](auto) { });
>> 
>> We could do that a bit earlier if it were a ContextDestructionObserver.
>> That might be better than to wait for GC.
> 
> I guess I can make it ContextDestructionObserver.

If we are going to make this ContextDestructionObserver, we might want to make FileSystemHandle ContextDestructionObserver as well (It currently does not keep track of context, so there is nothing to pass to FileSystemSyncAccessHandle.)
Try not introduce another another change in this, I will follow up in https://bugs.webkit.org/show_bug.cgi?id=231250.

[Sihui Liu]

Created attachment 440280 [details]
Patch

[youenn fablet]

Comment on attachment 440280 [details]
Patch

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

> Source/WebCore/Modules/filesystemaccess/FileSystemFileHandle.idl:34
> +    [EnabledBySetting=AccessHandleEnabled, Exposed=Worker] Promise<FileSystemSyncAccessHandle> createSyncAccessHandle();

If we want to expose FileSystemFileHandle in service workers, we need to make sure createSyncAccessHandle is not exposed.
Worth writing a test.

> LayoutTests/storage/filesystemaccess/resources/sync-access-handle-basics.js:5
> +description("This test checks basic funtionalities of FileSystemSyncAccessHandle.");

Seems like they could be WPT tests that we could at some point upstream

[Sihui Liu]

(In reply to youenn fablet from comment #16)
> Comment on attachment 440280 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440280&action=review
> 
> > Source/WebCore/Modules/filesystemaccess/FileSystemFileHandle.idl:34
> > +    [EnabledBySetting=AccessHandleEnabled, Exposed=Worker] Promise<FileSystemSyncAccessHandle> createSyncAccessHandle();
> 
> If we want to expose FileSystemFileHandle in service workers, we need to
> make sure createSyncAccessHandle is not exposed.
> Worth writing a test.

We seem to not support [Exposed=DedicatedWorker] on function yet; and partial interface is not supported on inherited class. I am not sure how to enable it in dedicated worker now; filed https://bugs.webkit.org/show_bug.cgi?id=231317 as a reminder for me.

> 
> > LayoutTests/storage/filesystemaccess/resources/sync-access-handle-basics.js:5
> > +description("This test checks basic funtionalities of FileSystemSyncAccessHandle.");
> 
> Seems like they could be WPT tests that we could at some point upstream

WPT have the AccessHandle tests, but they are dependent on some other interfaces like directory iteration.

[Sihui Liu]

Created attachment 440421 [details]
Patch for landing

[Sihui Liu]

Created attachment 440423 [details]
Patch for landing

[EWS]

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

[Sihui Liu]

Created attachment 440441 [details]
Patch for landing

[EWS]

Committed r283676 (242614@main): <https://commits.webkit.org/242614@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440441 [details].