Bug 231142

Created attachment 440013 [details] Patch

<rdar://problem/83848092>

Comment on attachment 440231 [details] Patch It would be cool if we could get async iterable in binding generator. Or maybe abstract a bit more the code so that the iterator is less file specific, look for JSDOMIteratorBase as an example. View in context: https://bugs.webkit.org/attachment.cgi?id=440231&action=review > Source/WebCore/ChangeLog:11 > + Since AsyncIterable is not supported in bindings code generator yet, introduce a new class I filed https://bugs.webkit.org/show_bug.cgi?id=231243. It would feel more robust to tackle this one first, but otherwise we should add very good test coverage for this. > Source/WebCore/ChangeLog:12 > + JSFileSystemDirectoryHandleIterator and impelement keys(), values(), and entries() as custom methods of s/impelement/implement/ > Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:103 > + Ref<FileSystemHandle> handle = FileSystemDirectoryHandle::create(String { name }, identifier, WTFMove(connection)); auto or one liner is not working? > Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:107 > + Ref<FileSystemHandle> handle = FileSystemFileHandle::create(String { name }, identifier, WTFMove(connection)); Not needed. > Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:108 > + return completionHandler(WTFMove(handle)); return not needed > Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandleIterator.cpp:51 > + if (!m_isInitialized) { Calling next twice in a row will go the !m_isInitialized code path. Are we sure this is ok? > Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandleIterator.cpp:97 > + if (m_type == Type::Value) We are converting key for no reason in that case. Might be just worth to call resolve<IDLDOMString> and resolve<IDLInterface...>> and special case the key+value code path. > Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandleIterator.cpp:104 > + return promise->resolveWithJSValue(jsPair); No need for return

Comment on attachment 440231 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440231&action=review >> Source/WebCore/ChangeLog:11 >> + Since AsyncIterable is not supported in bindings code generator yet, introduce a new class > > I filed https://bugs.webkit.org/show_bug.cgi?id=231243. > It would feel more robust to tackle this one first, but otherwise we should add very good test coverage for this. Yes, it should be more robust if time permits. I decided to just add a specialized class. If this can work, we should be able to turn it into template class. What kind of test coverage are you thinking? >> Source/WebCore/ChangeLog:12 >> + JSFileSystemDirectoryHandleIterator and impelement keys(), values(), and entries() as custom methods of > > s/impelement/implement/ Will change. >> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:103 >> + Ref<FileSystemHandle> handle = FileSystemDirectoryHandle::create(String { name }, identifier, WTFMove(connection)); > > auto or one liner is not working? Yes, it does not convert Ref<FileSystemDirectoryHandle> to Ref<FileSystemHandle> for completionHandler. >> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:108 >> + return completionHandler(WTFMove(handle)); > > return not needed Will remove. >> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandleIterator.cpp:51 >> + if (!m_isInitialized) { > > Calling next twice in a row will go the !m_isInitialized code path. Are we sure this is ok? According to FileSystemDirectoryHandleIterator, it should invoke the second next() after the first is done... >> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandleIterator.cpp:97 >> + if (m_type == Type::Value) > > We are converting key for no reason in that case. > Might be just worth to call resolve<IDLDOMString> and resolve<IDLInterface...>> and special case the key+value code path. Sure. >> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandleIterator.cpp:104 >> + return promise->resolveWithJSValue(jsPair); > > No need for return Will remove.

Created attachment 440246 [details] Patch

Created attachment 440247 [details] Patch

Created attachment 440249 [details] Patch

Created attachment 440254 [details] Patch

Created attachment 440277 [details] Patch

Created attachment 440278 [details] Patch

I suggest just copying existing JSDOMIterator implementation as JSDOMAsyncIterator first and modify it to meet the requirement instead of manually creating a new JS type. There are many subtle things we need to care about when creating a new JS types in WebCore. For example, all host function needs to be annotated via JSC_ANNOTATE_HOST_FUNCTION, or declared & defined by JSC_DECLARE_HOST_FUNCTION / JSC_DEFINE_HOST_FUNCTION. Appropriate IsoSubspace is created with needsDestruction etc. Currently, CodeGeneratorJS.pm & preprocess-idls.pl properly parses iterable annotations in IDL and generate proper JSDOMIterator for each types automatically. This avoids manual hand-crafting as much as possible to remove accidental mistakes of creating a new JS type. I think the code needed for JSDOMAsyncIterator would be the same or smaller than manually implementing async iterator. And it keeps the code clean, and less error-prone even when a new contract will be introduced in JSC side since we will change that too.

(In reply to Yusuke Suzuki from comment #22) > I suggest just copying existing JSDOMIterator implementation as > JSDOMAsyncIterator first and modify it to meet the requirement instead of > manually creating a new JS type. There are many subtle things we need to > care about when creating a new JS types in WebCore. > For example, all host function needs to be annotated via > JSC_ANNOTATE_HOST_FUNCTION, or declared & defined by > JSC_DECLARE_HOST_FUNCTION / JSC_DEFINE_HOST_FUNCTION. Appropriate > IsoSubspace is created with needsDestruction etc. > > Currently, CodeGeneratorJS.pm & preprocess-idls.pl properly parses iterable > annotations in IDL and generate proper JSDOMIterator for each types > automatically. This avoids manual hand-crafting as much as possible to > remove accidental mistakes of creating a new JS type. > > I think the code needed for JSDOMAsyncIterator would be the same or smaller > than manually implementing async iterator. And it keeps the code clean, and > less error-prone even when a new contract will be introduced in JSC side > since we will change that too. I will see if I can make a template class. For this patch I don't intend to make CodeGenerator change. This patch is not for supporting AsyncIterable in idl. I just need some helper class to support directory iteration quickly.

Comment on attachment 440254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440254&action=review Commented on JS related code. > Source/JavaScriptCore/runtime/JSPromise.cpp:121 > JSValue deferred = call(globalObject, newPromiseCapabilityFunction, callData, jsUndefined(), arguments); > RETURN_IF_EXCEPTION(scope, { }); > > + return deferred; Let's change it to RELEASE_AND_RETURN(scope, call(globalObject, newPromiseCapabilityFunction, callData, jsUndefined(), arguments)); > Source/JavaScriptCore/runtime/JSPromise.cpp:142 > + return convertCapabilityToDeferredData(globalObject, createNewPromiseCapability(globalObject, promiseConstructor)); createNewPromiseCapability can throw. So we should not pass it directly to convertCapabilityToDeferredData. Let's change it to the following. VM& vm = globalObject->vm(); auto scope = DECLARE_THROW_SCOPE(vm); JSValue capability = createNewPromiseCapability(globalObject, promiseConstructor); RETURN_IF_EXCEPTION(scope, { }); RELEASE_AND_RETURN(scope, convertCapabilityToDeferredData(globalObject, capability)); > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:37 > + Please file a bug replacing it with JSDOMAsyncIterator and add FIXME with the URL. I think manual implementation is too fragile... It should be replaced with JSDOMAsyncIterator soon. > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:60 > + return JSC::JSValue::encode(throwTypeError(globalObject, scope, "Cannot call next() on this object"_s)); Let's use throwVMTypeError. So, return throwVMTypeError(globalObject, scope, "Cannot call next() on this object"_s); > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:62 > + return JSC::JSValue::encode(iterator->next(*globalObject)); Let's change it to `RELEASE_AND_RETURN(scope, JSC::JSValue::encode(iterator->next(*globalObject));` because next can throw. > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:117 > + JSC::VM& vm = globalObject->vm(); Add auto scope = DECLARE_THROW_SCOPE(vm); > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:118 > + auto nextPromiseCapability = JSC::JSPromise::createNewPromiseCapability(globalObject, globalObject->promiseConstructor()); Add RETURN_IF_EXCEPTION(scope, nullptr); JS can throw an error anytime: terminated exception / stack-overflow / OOM-error. > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:119 > + auto data = JSC::JSPromise::convertCapabilityToDeferredData(globalObject, nextPromiseCapability); Add RETURN_IF_EXCEPTION(scope, nullptr); > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:121 > + data.promise->resolve(globalObject, createIteratorResultObject(globalObject, JSC::jsUndefined(), true)); Add RETURN_IF_EXCEPTION(scope, nullptr); > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:125 > + auto nextPromise = getNextIterationResult(*globalObject); Add RETURN_IF_EXCEPTION(scope, nullptr); > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:130 > + nextPromise->performPromiseThen(globalObject, boundedOnFulFilled, boundedOnRejected, nextPromiseCapability); Add RETURN_IF_EXCEPTION(scope, nullptr); > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:138 > + auto* iterator = JSC::jsDynamicCast<JSFileSystemDirectoryHandleIterator*>(vm, thisValue); It should be jsCast instead of jsDynamicCast if it is definitely an iterator. > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:149 > + JSC::VM& vm = globalObject.vm(); Add auto scope = DECLARE_THROW_SCOPE(vm); > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:151 > + m_ongoingPromise = DOMPromise::create(*this->globalObject(), *runNextSteps(&globalObject)); Since runNextSteps can throw, we should separate it to two~ statements. if (!m_ongoingPromise || !m_ongoingPromise->promise()) { auto* promise = runNextSteps(&globalObject); RETURN_IF_EXCEPTION(scope, { }); m_ongoingPromise = DOMPromise::create(*this->globalObject(), *promise); } else > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:153 > + auto afterOngoingPromiseCapability = JSC::JSPromise::createNewPromiseCapability(&globalObject, globalObject.promiseConstructor()); Add RETURN_IF_EXCEPTION(scope, { }); > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:154 > + auto data = JSC::JSPromise::convertCapabilityToDeferredData(&globalObject, afterOngoingPromiseCapability); Add RETURN_IF_EXCEPTION(scope, { }); > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:158 > + ongoingPromise->performPromiseThen(&globalObject, boundedOnSettled, boundedOnSettled, afterOngoingPromiseCapability); Add RETURN_IF_EXCEPTION(scope, { });

(In reply to Sihui Liu from comment #23) > (In reply to Yusuke Suzuki from comment #22) > > I suggest just copying existing JSDOMIterator implementation as > > JSDOMAsyncIterator first and modify it to meet the requirement instead of > > manually creating a new JS type. There are many subtle things we need to > > care about when creating a new JS types in WebCore. > > For example, all host function needs to be annotated via > > JSC_ANNOTATE_HOST_FUNCTION, or declared & defined by > > JSC_DECLARE_HOST_FUNCTION / JSC_DEFINE_HOST_FUNCTION. Appropriate > > IsoSubspace is created with needsDestruction etc. > > > > Currently, CodeGeneratorJS.pm & preprocess-idls.pl properly parses iterable > > annotations in IDL and generate proper JSDOMIterator for each types > > automatically. This avoids manual hand-crafting as much as possible to > > remove accidental mistakes of creating a new JS type. > > > > I think the code needed for JSDOMAsyncIterator would be the same or smaller > > than manually implementing async iterator. And it keeps the code clean, and > > less error-prone even when a new contract will be introduced in JSC side > > since we will change that too. > > I will see if I can make a template class. That's nice!! Auto-generated implementation is a key to keep things up-to-date to the JSC side changes. Because of various security constraints / optimizations, JSC side type definition protocol is changing. Auto-generated implementation will reduce errors. And it makes the implementation non-fragile against JSC side change :) > > For this patch I don't intend to make CodeGenerator change. This patch is > not for supporting AsyncIterable in idl. I just need some helper class to > support directory iteration quickly.

Comment on attachment 440254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440254&action=review >> Source/JavaScriptCore/runtime/JSPromise.cpp:121 >> + return deferred; > > Let's change it to > > RELEASE_AND_RETURN(scope, call(globalObject, newPromiseCapabilityFunction, callData, jsUndefined(), arguments)); Sure. >> Source/JavaScriptCore/runtime/JSPromise.cpp:142 >> + return convertCapabilityToDeferredData(globalObject, createNewPromiseCapability(globalObject, promiseConstructor)); > > createNewPromiseCapability can throw. So we should not pass it directly to convertCapabilityToDeferredData. > Let's change it to the following. > > VM& vm = globalObject->vm(); > auto scope = DECLARE_THROW_SCOPE(vm); > > JSValue capability = createNewPromiseCapability(globalObject, promiseConstructor); > RETURN_IF_EXCEPTION(scope, { }); > RELEASE_AND_RETURN(scope, convertCapabilityToDeferredData(globalObject, capability)); Sure. >> Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:37 >> + > > Please file a bug replacing it with JSDOMAsyncIterator and add FIXME with the URL. > I think manual implementation is too fragile... It should be replaced with JSDOMAsyncIterator soon. I will add JSDOMAsyncIterator and make JSFileSystemDirectoryHandleIterator inherit it in next patch >> Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:60 >> + return JSC::JSValue::encode(throwTypeError(globalObject, scope, "Cannot call next() on this object"_s)); > > Let's use throwVMTypeError. So, > > return throwVMTypeError(globalObject, scope, "Cannot call next() on this object"_s); Sure. >> Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:62 >> + return JSC::JSValue::encode(iterator->next(*globalObject)); > > Let's change it to `RELEASE_AND_RETURN(scope, JSC::JSValue::encode(iterator->next(*globalObject));` because next can throw. Sure. >> Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:117 >> + JSC::VM& vm = globalObject->vm(); > > Add auto scope = DECLARE_THROW_SCOPE(vm); Okay. >> Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:118 >> + auto nextPromiseCapability = JSC::JSPromise::createNewPromiseCapability(globalObject, globalObject->promiseConstructor()); > > Add RETURN_IF_EXCEPTION(scope, nullptr); > JS can throw an error anytime: terminated exception / stack-overflow / OOM-error. Okay. >> Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:119 >> + auto data = JSC::JSPromise::convertCapabilityToDeferredData(globalObject, nextPromiseCapability); > > Add RETURN_IF_EXCEPTION(scope, nullptr); Okay. >> Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:121 >> + data.promise->resolve(globalObject, createIteratorResultObject(globalObject, JSC::jsUndefined(), true)); > > Add RETURN_IF_EXCEPTION(scope, nullptr); Okay. >> Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:125 >> + auto nextPromise = getNextIterationResult(*globalObject); > > Add RETURN_IF_EXCEPTION(scope, nullptr); Okay. >> Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:130 >> + nextPromise->performPromiseThen(globalObject, boundedOnFulFilled, boundedOnRejected, nextPromiseCapability); > > Add RETURN_IF_EXCEPTION(scope, nullptr); Okay. >> Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:138 >> + auto* iterator = JSC::jsDynamicCast<JSFileSystemDirectoryHandleIterator*>(vm, thisValue); > > It should be jsCast instead of jsDynamicCast if it is definitely an iterator. Okay. >> Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:149 >> + JSC::VM& vm = globalObject.vm(); > > Add auto scope = DECLARE_THROW_SCOPE(vm); Okay. >> Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:151 >> + m_ongoingPromise = DOMPromise::create(*this->globalObject(), *runNextSteps(&globalObject)); > > Since runNextSteps can throw, we should separate it to two~ statements. > > if (!m_ongoingPromise || !m_ongoingPromise->promise()) { > auto* promise = runNextSteps(&globalObject); > RETURN_IF_EXCEPTION(scope, { }); > m_ongoingPromise = DOMPromise::create(*this->globalObject(), *promise); > } else Got it. >> Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:153 >> + auto afterOngoingPromiseCapability = JSC::JSPromise::createNewPromiseCapability(&globalObject, globalObject.promiseConstructor()); > > Add RETURN_IF_EXCEPTION(scope, { }); Okay. >> Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:154 >> + auto data = JSC::JSPromise::convertCapabilityToDeferredData(&globalObject, afterOngoingPromiseCapability); > > Add RETURN_IF_EXCEPTION(scope, { }); Okay. >> Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:158 >> + ongoingPromise->performPromiseThen(&globalObject, boundedOnSettled, boundedOnSettled, afterOngoingPromiseCapability); > > Add RETURN_IF_EXCEPTION(scope, { }); Okay.

Created attachment 440468 [details] Patch

Created attachment 440478 [details] Patch

Created attachment 440484 [details] Patch

Comment on attachment 440484 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440484&action=review Commented. > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:119 > + virtual JSC::JSBoundFunction* createOnSettledFunction(JSC::JSGlobalObject*) = 0; > + virtual JSC::JSBoundFunction* createOnFulfilledFunction(JSC::JSGlobalObject*) = 0; > + virtual JSC::JSBoundFunction* createOnRejectedFunction(JSC::JSGlobalObject*) = 0; This is not OK. JSObject cannot use virtual. > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:133 > + virtual ~JSDOMAsyncIteratorBase() = default; JSObject cannot use virtual.

Created attachment 440518 [details] Patch

Comment on attachment 440518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440518&action=review > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:231 > + ongoingPromise->performPromiseThen(&lexicalGlobalObject, onSettled, onSettled, afterOngoingPromiseCapability); Since we have a DOMPromise, can we use DOMPromise::whenPromiseIsSettled?

(In reply to youenn fablet from comment #32) > Comment on attachment 440518 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=440518&action=review > > > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:231 > > + ongoingPromise->performPromiseThen(&lexicalGlobalObject, onSettled, onSettled, afterOngoingPromiseCapability); > > Since we have a DOMPromise, can we use DOMPromise::whenPromiseIsSettled? The spec says to use performPromiseThen to chain promises; I am not sure if whenPromiseIsSettled is exactly the same as performPromiseThen. And below here we do need performPromiseThen to set both onFulfilled and onRejected, so I just use performPromiseThen here to ensure we match spec.

I believe these are roughly the same. Introducing performPromiseThen is a good idea since it is used by WebIDL directly, I would update DOMPromise::whenPromiseIsSettled to use it.

(In reply to youenn fablet from comment #34) > I believe these are roughly the same. > Introducing performPromiseThen is a good idea since it is used by WebIDL > directly, I would update DOMPromise::whenPromiseIsSettled to use it. DOMPromise::whenPromiseIsSettled uses then() method (https://tc39.es/ecma262/#sec-promise.prototype.then), which is bit different from PerformPromiseThen (https://tc39.es/ecma262/#sec-performpromisethen), though then() actually invokes PerformPromiseThen() in implementation. We may introduce DOMPromise::whenPromiseIsSettledWithResultCapability as a follow-up I guess, if there are other usage of it.

Created attachment 440535 [details] Patch

Comment on attachment 440535 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440535&action=review One comment. > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:41 > +JSC_ANNOTATE_HOST_FUNCTION(FileSystemDirectoryHandleIteratorPrototypeNext, FileSystemDirectoryHandleIteratorPrototype::next); This should be JSC_ANNOTATE_HOST_FUNCTION(JSFileSystemDirectoryHandleIteratorPrototypeNext, JSFileSystemDirectoryHandleIteratorPrototype::next);

(In reply to Yusuke Suzuki from comment #37) > Comment on attachment 440535 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=440535&action=review > > One comment. > > > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.cpp:41 > > +JSC_ANNOTATE_HOST_FUNCTION(FileSystemDirectoryHandleIteratorPrototypeNext, FileSystemDirectoryHandleIteratorPrototype::next); > > This should be > > JSC_ANNOTATE_HOST_FUNCTION(JSFileSystemDirectoryHandleIteratorPrototypeNext, > JSFileSystemDirectoryHandleIteratorPrototype::next); ah nice catch!

Created attachment 440571 [details] Patch

Comment on attachment 440571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440571&action=review > Source/JavaScriptCore/runtime/JSPromise.cpp:255 > +void JSPromise::performPromiseThen(JSGlobalObject* globalObject, JSFunction* onFulFilled, JSFunction* onRejected, JSValue resultCapability) Worth checking with JSC guys but it seems like we should try passing references instead of pointers here. > Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:126 > + m_isWaitingForResult = true; We could probably centralise m_isWaitingForResult use in next, by doing something like: auto callback = [this, protectedThis = Ref { *this }, completionHandler = WTFMove(completionHandler)](...) { m_isWaitingForResult = false; completionHandler(WTFMove(result)); } > Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:150 > + return completionHandler(WTFMove(result)); Can we just write it completionHandler(Result { }); > Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:153 > + auto key = m_keys[m_index++]; This probably triggers a ref call, how about not cresting a temp variable. > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:44 > +// struct IteratorTraits { Worth adding a reference to WebIDL spec/ WebIDL algorithms. > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:117 > + JSC::EncodedJSValue reject(JSC::JSGlobalObject*, JSC::JSValue); Some of these functions can probably be made private for now. Where it make sense, I would make them take a JSGlobalObject& > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:134 > + RefPtr<DOMPromise> m_ongoingPromise; We do not have https://webidl.spec.whatwg.org/#default-asynchronous-iterator-object-is-finished here. Would it make sense to introduce it to be closer to spec? > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:137 > +inline JSC::JSValue jsPair(JSC::JSGlobalObject&, JSDOMGlobalObject& globalObject, JSC::JSValue value1, JSC::JSValue value2) Probably worth sharing with JSDOMIterator.h > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:181 > +inline EnableIfSet<IteratorTraits, JSC::JSValue> convertToJS(JSC::JSGlobalObject& globalObject, JSDOMGlobalObject& domGlobalObject, IteratorValue& value, IteratorTraits, IterationKind kind) Might be worth trying to share this code with JSDMOIterator. The only difference seems to be JSDOMAsyncIteratorType vs. JSDOMIteratorType maybe we should suppress this difference > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:227 > + auto onSettled = castedThis->createOnSettledFunction(&lexicalGlobalObject); Why do we need to call a castedThis specific createOnSettledFunction. It does not seem like it is specific to FileHandle since we basically want to call runNextSteps when the promise is settled. If it is specific, I would hope we can use m_iterator directly somehow. > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:230 > + JSC::JSPromise* ongoingPromise = m_ongoingPromise->promise(); auto* > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:241 > +JSC::JSPromise* JSDOMAsyncIteratorBase<JSWrapper, IteratorTraits, JSIterator>::runNextSteps(JSC::JSGlobalObject* globalObject) Can probably take a JSGlobalObject& > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:262 > + JSIterator* castedThis = jsDynamicCast<JSIterator*>(vm, this); auto > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:266 > + auto onRejected = castedThis->createOnRejectedFunction(globalObject); Ditto here, why do we need special castedThis functions? Cannot we create them here directly? > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:283 > + deferred->resolveWithJSValue(JSC::jsUndefined()); I would return early here. > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:285 > + auto traits = IteratorTraits { }; IteratorTraits traits > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:296 > + return deferred->resolveWithJSValue(JSC::jsUndefined()); deferred->resolve() > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:298 > + > + return deferred->resolveWithJSValue(convertToJS(*globalObject, *globalObject, *resultValue, traits, kind)); no need for return > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:314 > + m_ongoingPromise = nullptr; Is it correct? If next is called twice synchronously, m_ongoingPromise will be changed twice and the one we nullify might not be the same. > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:320 > + return JSC::JSValue::encode(JSC::createIteratorResultObject(globalObject, result, false)); no need for return > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleCustom.cpp:36 > + return iteratorCreate<JSFileSystemDirectoryHandleIterator>(*this, JSC::IterationKind::Entries); Would be nice if we could refactor the patch to just use JSDOMAsyncIteratorBase here. Looking at JSFileSystemDirectoryHandleIterator, it does not seem like it has any file specific functionality. > Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:228 > + auto result = requestCreateHandle(connection, FileSystemStorageHandle::Type::File, String { name }, false); Worth changing createIfNecessary last parameter to an enum or to have something like bool createIfNecessary = false; auto result = ...., createIfNecessary) > Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:232 > + result = requestCreateHandle(connection, FileSystemStorageHandle::Type::Directory, WTFMove(name), false); It seems we should change requestCreateHandle to take an optional FileSystemStorageHandle::Type, that way, we would make just one call here. It seems strange for instance to only return the second error and not the first one. > LayoutTests/ChangeLog:12 > + * storage/filesystemaccess/directory-handle-iteration.html: Added. Can we run them in workers as well? > LayoutTests/storage/filesystemaccess/resources/directory-handle-iteration.js:40 > + for await (key of rootHandle.keys()) { I think it is worth checking more than for of loop. Worth checking what happens when calling next directly and so on. Worth checking the prototype...

Comment on attachment 440571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440571&action=review >> Source/JavaScriptCore/runtime/JSPromise.cpp:255 >> +void JSPromise::performPromiseThen(JSGlobalObject* globalObject, JSFunction* onFulFilled, JSFunction* onRejected, JSValue resultCapability) > > Worth checking with JSC guys but it seems like we should try passing references instead of pointers here. You mean JSGlobalObject*? Currently JSPromise functions all use pointer so I just follow the pattern. >> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:126 >> + m_isWaitingForResult = true; > > We could probably centralise m_isWaitingForResult use in next, by doing something like: > auto callback = [this, protectedThis = Ref { *this }, completionHandler = WTFMove(completionHandler)](...) { > m_isWaitingForResult = false; > completionHandler(WTFMove(result)); > } Sure. >> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:150 >> + return completionHandler(WTFMove(result)); > > Can we just write it completionHandler(Result { }); Sure. >> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:153 >> + auto key = m_keys[m_index++]; > > This probably triggers a ref call, how about not cresting a temp variable. Sure, will remove. >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:44 >> +// struct IteratorTraits { > > Worth adding a reference to WebIDL spec/ WebIDL algorithms. I copied this part from JSDOMIterator. For algorithm, you mean adding https://webidl.spec.whatwg.org/#idl-async-iterable? >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:117 >> + JSC::EncodedJSValue reject(JSC::JSGlobalObject*, JSC::JSValue); > > Some of these functions can probably be made private for now. > Where it make sense, I would make them take a JSGlobalObject& Sure. >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:134 >> + RefPtr<DOMPromise> m_ongoingPromise; > > We do not have https://webidl.spec.whatwg.org/#default-asynchronous-iterator-object-is-finished here. > Would it make sense to introduce it to be closer to spec? When it reaches to end we set m_iterator to null, so we may just check m_iterator? >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:137 >> +inline JSC::JSValue jsPair(JSC::JSGlobalObject&, JSDOMGlobalObject& globalObject, JSC::JSValue value1, JSC::JSValue value2) > > Probably worth sharing with JSDOMIterator.h Yes, will file a bug. >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:181 >> +inline EnableIfSet<IteratorTraits, JSC::JSValue> convertToJS(JSC::JSGlobalObject& globalObject, JSDOMGlobalObject& domGlobalObject, IteratorValue& value, IteratorTraits, IterationKind kind) > > Might be worth trying to share this code with JSDMOIterator. > The only difference seems to be JSDOMAsyncIteratorType vs. JSDOMIteratorType maybe we should suppress this difference The other difference is: 1. this function takes a lock (I am not sure why JSDMOIterator does not need one, but toJS crashes if I don't add the lock) 2. This is a global function and asJS in JSDMOIterator is a class member function. (We may share the code I think; will need to need another change in JSDOMIterator to adopt.) >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:227 >> + auto onSettled = castedThis->createOnSettledFunction(&lexicalGlobalObject); > > Why do we need to call a castedThis specific createOnSettledFunction. It does not seem like it is specific to FileHandle since we basically want to call runNextSteps when the promise is settled. > If it is specific, I would hope we can use m_iterator directly somehow. Because we can only create JSFunction with static function, and the static function cannot deduct the type of the template class (so we cannot define createOnSettledFunction in JSDOMAsyncIteratorBase)? (We need JSFunction here to feed performPromiseThen.) Do you have better idea about this? (I am happy to try) >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:230 >> + JSC::JSPromise* ongoingPromise = m_ongoingPromise->promise(); > > auto* Sure. >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:241 >> +JSC::JSPromise* JSDOMAsyncIteratorBase<JSWrapper, IteratorTraits, JSIterator>::runNextSteps(JSC::JSGlobalObject* globalObject) > > Can probably take a JSGlobalObject& Sure. >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:262 >> + JSIterator* castedThis = jsDynamicCast<JSIterator*>(vm, this); > > auto Sure. >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:266 >> + auto onRejected = castedThis->createOnRejectedFunction(globalObject); > > Ditto here, why do we need special castedThis functions? > Cannot we create them here directly? Like mentioned above. >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:283 >> + deferred->resolveWithJSValue(JSC::jsUndefined()); > > I would return early here. Sure. >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:285 >> + auto traits = IteratorTraits { }; > > IteratorTraits traits Will change. >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:296 >> + return deferred->resolveWithJSValue(JSC::jsUndefined()); > > deferred->resolve() Okay. >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:298 >> + return deferred->resolveWithJSValue(convertToJS(*globalObject, *globalObject, *resultValue, traits, kind)); > > no need for return Will remove. >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:314 >> + m_ongoingPromise = nullptr; > > Is it correct? > If next is called twice synchronously, m_ongoingPromise will be changed twice and the one we nullify might not be the same. Yes, this is the fulfill function. So if there is a second promise created by a second next() call, result of first promise should be sent to settle function, instead of fulfill or reject function, meaning the result is ignored. Spec: https://webidl.spec.whatwg.org/#es-asynchronous-iterator-prototype-object - step 9. >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:320 >> + return JSC::JSValue::encode(JSC::createIteratorResultObject(globalObject, result, false)); > > no need for return why? >> Source/WebCore/bindings/js/JSFileSystemDirectoryHandleCustom.cpp:36 >> + return iteratorCreate<JSFileSystemDirectoryHandleIterator>(*this, JSC::IterationKind::Entries); > > Would be nice if we could refactor the patch to just use JSDOMAsyncIteratorBase here. > Looking at JSFileSystemDirectoryHandleIterator, it does not seem like it has any file specific functionality. The reason I made JSFileSystemDirectoryHandleIterator: 1. we need it to create functions for performPromiseThen in JSDOMAsyncIteratorBase (maybe you have better idea on how to do this) 2. subspaceforimpl uses a specified subspace for JSFileSystemDirectoryHandleIterator 2. it's mimicking what we will do with code generator support (adding a new class for async iterate interface like what we did for iterable interface today), so it's easier to add support by moving JSFileSystemDirectoryHandleIterator code to code generator later? >> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:228 >> + auto result = requestCreateHandle(connection, FileSystemStorageHandle::Type::File, String { name }, false); > > Worth changing createIfNecessary last parameter to an enum or to have something like > bool createIfNecessary = false; > auto result = ...., createIfNecessary) Sure, I will use a bool now to avoid touching all the other call sites here. >> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:232 >> + result = requestCreateHandle(connection, FileSystemStorageHandle::Type::Directory, WTFMove(name), false); > > It seems we should change requestCreateHandle to take an optional FileSystemStorageHandle::Type, that way, we would make just one call here. > It seems strange for instance to only return the second error and not the first one. Sure. >> LayoutTests/ChangeLog:12 >> + * storage/filesystemaccess/directory-handle-iteration.html: Added. > > Can we run them in workers as well? Yes, will add. >> LayoutTests/storage/filesystemaccess/resources/directory-handle-iteration.js:40 >> + for await (key of rootHandle.keys()) { > > I think it is worth checking more than for of loop. > Worth checking what happens when calling next directly and so on. > Worth checking the prototype... Will add.

Comment on attachment 440571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440571&action=review >>> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:153 >>> + auto key = m_keys[m_index++]; >> >> This probably triggers a ref call, how about not cresting a temp variable. > > Sure, will remove. hmm the key is used twice below; do you mean just replacing them with m_keys[m_index] and add m_index after m_source->getHandle...? >>> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:117 >>> + JSC::EncodedJSValue reject(JSC::JSGlobalObject*, JSC::JSValue); >> >> Some of these functions can probably be made private for now. >> Where it make sense, I would make them take a JSGlobalObject& > > Sure. Hmm we can't since this is used by the static functions of JJSFileSystemDirectoryHandleIterator. >>> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:137 >>> +inline JSC::JSValue jsPair(JSC::JSGlobalObject&, JSDOMGlobalObject& globalObject, JSC::JSValue value1, JSC::JSValue value2) >> >> Probably worth sharing with JSDOMIterator.h > > Yes, will file a bug. Filed: https://bugs.webkit.org/show_bug.cgi?id=231437 >>> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:296 >>> + return deferred->resolveWithJSValue(JSC::jsUndefined()); >> >> deferred->resolve() > > Okay. resolve() cannot be called with JSValue...

Created attachment 440648 [details] Patch

Comment on attachment 440571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440571&action=review >>> Source/JavaScriptCore/runtime/JSPromise.cpp:255 >>> +void JSPromise::performPromiseThen(JSGlobalObject* globalObject, JSFunction* onFulFilled, JSFunction* onRejected, JSValue resultCapability) >> >> Worth checking with JSC guys but it seems like we should try passing references instead of pointers here. > > You mean JSGlobalObject*? Currently JSPromise functions all use pointer so I just follow the pattern. Yeah, I like the current style. Using WebCore DOM style partially looks inconsistent to JSC dev.

Comment on attachment 440648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440648&action=review > Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:149 > +{ We can add ASSERT(m_isInitialized); > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:178 > +inline EnableIfSet<IteratorTraits, JSC::JSValue> convertToJS(JSC::JSGlobalObject& globalObject, JSDOMGlobalObject& domGlobalObject, IteratorValue& value, IteratorTraits, IterationKind kind) Do we need IteratorTraits? > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:213 > + m_ongoingPromise = DOMPromise::create(*this->globalObject(), *promise); I would tend to do a return here. > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:259 > + JSIterator* castedThis = jsDynamicCast<JSIterator*>(vm, this); auto* > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:263 > + auto onRejected = castedThis->createOnRejectedFunction(&globalObject); It seems we could reduce JSFileSystemDirectoryHandleIterator code quite a bit if we were creating the functions here. One possibility is to create JSNativeStdFunction functions, these functions would need to keep a ref to the JSIterator. Let's look at that as a follow-up, maybe JS folks will have ideas (or we keep the current approach). > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:280 > + deferred->resolveWithJSValue(JSC::jsUndefined()); I think deferred->resolve() will be equivalent to deferred->resolveWithJSValue(JSC::jsUndefined()); > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:284 > + IteratorTraits traits; Why do we need to create it here then capture it in the lambda? Can we move it in the lambda itself or even remove it entirely? > Source/WebCore/bindings/js/JSDOMAsyncIterator.h:295 > + return deferred->resolveWithJSValue(JSC::jsUndefined()); Ditto for deferred->resolve() which should be equivalent and shorter. > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.h:42 > +class JSFileSystemDirectoryHandleIterator final : public JSDOMAsyncIteratorBase<JSFileSystemDirectoryHandle, JSFileSystemDirectoryHandleIteratorTraits, JSFileSystemDirectoryHandleIterator> { Maybe add a FIXME to say that this code should be removed when binding generator supports async iterables > Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.h:70 > + JSC::JSBoundFunction* createOnRejectedFunction(JSC::JSGlobalObject*); Seems better to use refs where possible > Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:49 > + ASSERT(m_type != FileSystemStorageHandle::Type::Any); I would tend to keep a switch and use ASSERT_NOT_REACHED for FileSystemStorageHandle::Type::Any > Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:236 > + return makeUnexpected(result.error()); Would tend to write it as: if (!result) return makeUnexpected(result.error()); > LayoutTests/storage/filesystemaccess/resources/directory-handle-iteration.js:139 > + } Nice tests. Maybe we cannot right now but I think we should add tests exercising promise rejection.

> For algorithm, you mean adding > https://webidl.spec.whatwg.org/#idl-async-iterable? Right, and also if possible more precise links around https://webidl.spec.whatwg.org/#es-asynchronous-iterable, for instance https://webidl.spec.whatwg.org/#es-asynchronous-iterator-prototype-object to clearly match algorithms with implementation.

Comment on attachment 440648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440648&action=review >> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:149 >> +{ > > We can add ASSERT(m_isInitialized); Sure. >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:213 >> + m_ongoingPromise = DOMPromise::create(*this->globalObject(), *promise); > > I would tend to do a return here. Sure. >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:259 >> + JSIterator* castedThis = jsDynamicCast<JSIterator*>(vm, this); > > auto* Will add. >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:263 >> + auto onRejected = castedThis->createOnRejectedFunction(&globalObject); > > It seems we could reduce JSFileSystemDirectoryHandleIterator code quite a bit if we were creating the functions here. > One possibility is to create JSNativeStdFunction functions, these functions would need to keep a ref to the JSIterator. > Let's look at that as a follow-up, maybe JS folks will have ideas (or we keep the current approach). Yusuke suggested me adding JSIterator to the template, but maybe JSNativeStdFunction will work too. Sounds good to make a follow-up. >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:280 >> + deferred->resolveWithJSValue(JSC::jsUndefined()); > > I think deferred->resolve() will be equivalent to deferred->resolveWithJSValue(JSC::jsUndefined()); Oh I see; I thought you mean deferred->resolve(JSC::jsUndefined()); will change. >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:284 >> + IteratorTraits traits; > > Why do we need to create it here then capture it in the lambda? > Can we move it in the lambda itself or even remove it entirely? Yes we can move it to lambda. The callback does not hold reference to the JSObject, and convertToJS() is a global function (not a member function of JSDOMAsyncIteratorBase), so I passed the IteratorTraits explicitly. >> Source/WebCore/bindings/js/JSDOMAsyncIterator.h:295 >> + return deferred->resolveWithJSValue(JSC::jsUndefined()); > > Ditto for deferred->resolve() which should be equivalent and shorter. Will change. >> Source/WebCore/bindings/js/JSFileSystemDirectoryHandleIterator.h:42 >> +class JSFileSystemDirectoryHandleIterator final : public JSDOMAsyncIteratorBase<JSFileSystemDirectoryHandle, JSFileSystemDirectoryHandleIteratorTraits, JSFileSystemDirectoryHandleIterator> { > > Maybe add a FIXME to say that this code should be removed when binding generator supports async iterables Sure. >> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:49 >> + ASSERT(m_type != FileSystemStorageHandle::Type::Any); > > I would tend to keep a switch and use ASSERT_NOT_REACHED for FileSystemStorageHandle::Type::Any Sure. >> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:236 >> + return makeUnexpected(result.error()); > > Would tend to write it as: > if (!result) > return makeUnexpected(result.error()); Will change. >> LayoutTests/storage/filesystemaccess/resources/directory-handle-iteration.js:139 >> + } > > Nice tests. > Maybe we cannot right now but I think we should add tests exercising promise rejection. Sure.

Created attachment 440741 [details] Patch for landing

Committed r283881 (242758@main): <https://commits.webkit.org/242758@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 440741 [details].