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

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted 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 440648: Patch

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




--- Comment #45 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 440648
  --> https://bugs.webkit.org/attachment.cgi?id=440648
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:13
9
> +    }

Nice tests.
Maybe we cannot right now but I think we should add tests exercising promise
rejection.