[webkit-reviews] review canceled: [Bug 231142] Add support for iterating FileSystemDirectoryHandle : [Attachment 440571] Patch

Fri Oct 8 08:49:40 PDT 2021

Sihui Liu <sihui_liu at apple.com> has canceled Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 231142: Add support for iterating FileSystemDirectoryHandle
https://bugs.webkit.org/show_bug.cgi?id=231142

Attachment 440571: Patch

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

--- Comment #41 from Sihui Liu <sihui_liu at apple.com> ---
Comment on attachment 440571
  --> https://bugs.webkit.org/attachment.cgi?id=440571
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-finishe
d 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.