[webkit-reviews] review canceled: [Bug 230805] Make File System Access API available in Worker : [Attachment 439325] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Sep 27 12:06:30 PDT 2021


Sihui Liu <sihui_liu at apple.com> has canceled Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 230805: Make File System Access API available in Worker
https://bugs.webkit.org/show_bug.cgi?id=230805

Attachment 439325: Patch

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




--- Comment #11 from Sihui Liu <sihui_liu at apple.com> ---
Comment on attachment 439325
  --> https://bugs.webkit.org/attachment.cgi?id=439325
Patch

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

I will go split the patch into smaller ones for easier review.

>> Source/WTF/wtf/ObjectIdentifier.h:113
>> +	ObjectIdentifier isolatedCopy() const
> 
> Why do we need this one?

I used CrossThreadCopy for the result in WorkerFileSystemStorageConnection, and
ExceptionOr.h will call isolatedCopy on the result value.

An alternative is to add template<> struct CrossThreadCopierBase... like for
ExceptionOr<void> in ExceptionOr.h I guess

>> Source/WebCore/Modules/filesystemaccess/FileSystemHandle.cpp:40
>> +	, m_connection(connection)
> 
> WTFMove

Will change.

>>
Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:5
1
>> +	m_scope->addFileSystemStorageConnection(m_identifier, *this);
> 
> There should be a need for one connection per worker, so we can use the
ScriptContextExecutionIdentifier to identify the connection if needed.

I made it possible that there can be multiple connections per worker because
WorkerFileSystemStorageConnection is affiliated with network process
connection. 
E.g. If network process connection is lost, the
WorkerFileSystemStorageConnection whose mainThreadConnection is using the
NetworkProcessConnection will be inactive . New handles created later with
StorageManager.getDirectory will has a new WorkerFileSystemStorageConnection
with new mainThreadConnection.

>>
Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:5
3
>> +	    m_mainThreadConnection = WTFMove(connection);
> 
> I think we should be able to get the connection of the owner of the worker
here.
> Either we do callOnMainThreadAndWait and we retrieve a process wide
connection.
> Or we do a postTaskToOwner like we do in WorkerThreadableLoader and we can
retrieve the connection from the worker's Document.

Do you mean we should not keep the m_mainThreadConnection in
WorkerFileSystemStorageConnection?

>>
Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:9
6
>> +		   
downcast<WorkerGlobalScope>(scope).fileSystemStorageConnection(connectionIdenti
fier)->didIsSameEntry(callbackIdentifier, WTFMove(result));
> 
> If we have just one connection per worker, we should be able to do without
connectionIdentifier.

Like explained above; current design is one connection per handle family (root
handle is the one created from StorageManager.getDirectory())

>>
Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.h:59
>> +	RefPtr<WorkerGlobalScope> m_scope;
> 
> I would use a WeakPtr<WorkerGlobalScope> instead of a Ref.
> This will reduce the issue of a ref cycle.
> This should be safe given we will always use m_scope from worker thread.
> And we can pass the weak ptr as part of callbacks as well.

Sure.


More information about the webkit-reviews mailing list