...
Created attachment 439022 [details] Patch
Created attachment 439044 [details] Patch
<rdar://problem/83448115>
Created attachment 439151 [details] Patch
Created attachment 439372 [details] Patch
Created attachment 439374 [details] Patch
Comment on attachment 439374 [details] Patch Looks good to me overall, a few improvement ideas below. View in context: https://bugs.webkit.org/attachment.cgi?id=439374&action=review > Source/WebCore/Modules/storage/StorageConnection.h:37 > +class StorageConnection : public ThreadSafeRefCounted<StorageConnection> { I would mark it as DestructionThread::MainRunLoop for extra safety. > Source/WebCore/Modules/storage/WorkerStorageConnection.cpp:52 > + callOnMainThreadAndWait([mainThreadConnection = WTFMove(m_mainThreadConnection)]() mutable { }); Not needed with DestructionThread::Main/MainRunLoop > Source/WebCore/Modules/storage/WorkerStorageConnection.cpp:66 > + if (!m_scope) I would add ASSERT(m_scope). > Source/WebCore/Modules/storage/WorkerStorageConnection.cpp:76 > + downcast<WorkerStorageConnection>(*storageConnection).didGetPersisted(callbackIdentifier, result); Ideally, we would not need that, WorkerGlobalScope could just return a WorkerStorageConnection, which would remove the need for isWorker(). > Source/WebCore/Modules/storage/WorkerStorageConnection.cpp:91 > + if (!m_scope) Ditto here > Source/WebCore/Modules/storage/WorkerStorageConnection.h:40 > + void didGetPersisted(uint64_t callbackIdentifier, bool result); Should be private > Source/WebCore/Modules/storage/WorkerStorageConnection.h:43 > + WorkerStorageConnection(WorkerGlobalScope&); explicit > Source/WebCore/Modules/storage/WorkerStorageConnection.h:50 > + RefPtr<WorkerGlobalScope> m_scope; WeakPtr to cut ref cycles? > Source/WebCore/workers/WorkerGlobalScope.cpp:221 > +StorageConnection* WorkerGlobalScope::storageConnection() Should be &, ideally WorkerStorageConnection& > Source/WebCore/workers/WorkerGlobalScope.h:85 > + StorageConnection* storageConnection() final; Not sure whether we need a virtual storageConnection(), maybe we can have one for Document and one for WorkerGlobalScope, but not on ScriptExecutionContext.
Created attachment 439385 [details] Patch
Created attachment 439396 [details] Patch
Created attachment 439432 [details] Patch
Comment on attachment 439432 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439432&action=review > Source/WebCore/Modules/storage/WorkerStorageConnection.cpp:46 > + m_mainThreadConnection = scope.thread().workerLoaderProxy().storageConnection(); storageConnection() is currently a pointer so m_mainThreadConnection can be nullptr for instance in getPersisted. Maybe we can guarantee to have a reference here which will make sure m_mainThreadConnection is not null. > Source/WebCore/Modules/storage/WorkerStorageConnection.cpp:71 > + callOnMainThread([callbackIdentifier, workerThread = Ref { m_scope->thread() }, mainThreadConnection = m_mainThreadConnection, origin = origin.isolatedCopy()]() mutable { Since we are taking a ref to the worker thread, another strategy is to not have m_mainThreadConnection and retrieve it from worker thread loader proxy each time we need it from main thread. I would tend to do this plus a null check on the connection. > Source/WebCore/Modules/storage/WorkerStorageConnection.cpp:72 > + auto mainThreadCallback = [callbackIdentifier, workerThread = WTFMove(workerThread)](auto result) mutable { Let's use bool here. If in the future, we want to have ExceptionOr<bool>, we will need to change the code and maybe isolate copy it.
Created attachment 439492 [details] Patch for landing
Committed r283184 (242232@main): <https://commits.webkit.org/242232@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 439492 [details].