Bug 176091 – Implement FileSystemDirectoryReader.readEntries()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Implement FileSystemDirectoryReader.readEntries():
- https://wicg.github.io/entries-api/#dom-filesystemdirectoryreader-readentries

[Chris Dumez]

Created attachment 319342 [details]
WIP Patch

[Build Bot]

Attachment 319342 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/entriesapi/DOMFileSystem.cpp:107:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 319344 [details]
WIP Patch

[Build Bot]

Attachment 319344 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/entriesapi/DOMFileSystem.cpp:107:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 319369 [details]
WIP Patch

[Chris Dumez]

Created attachment 319383 [details]
WIP Patch

[Radar WebKit Bug Importer]

<rdar://problem/34168015>

[Chris Dumez]

Created attachment 319385 [details]
Patch

[Andreas Kling]

Comment on attachment 319385 [details]
Patch

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

r=me

> Source/WebCore/Modules/entriesapi/DOMFileSystem.cpp:53
> +    Vector<String> childPaths = listDirectory(fullPath, "*");

auto

> Source/WebCore/Modules/entriesapi/DOMFileSystem.cpp:119
> +    Vector<String> components;
> +    virtualPath.split('/', false /* allowEmpty */, components);

auto + overload of split() would look nice :)

> Source/WebCore/Modules/entriesapi/DOMFileSystem.cpp:157
> +        callOnMainThread([this, completionHandler = WTFMove(completionHandler), listedChildren = CrossThreadCopier<ExceptionOr<Vector<ListedChild>>>::copy(listedChildren), directoryPtr]() mutable {

It would be cool to have a makeCrossThreadCopy() helper.

> Source/WebCore/Modules/entriesapi/FileSystemDirectoryReader.cpp:80
> +    setPendingActivity(this);

I think it would be nice to use a RAII helper object for this so we don't have to unset in every possible code path.

> LayoutTests/editing/pasteboard/datatransfer-items-drop-directoryReader-root.html:16
> +function moveMouseToCenterOfElement(element) {
> +    var centerX = element.offsetLeft + element.offsetWidth / 2;
> +    var centerY = element.offsetTop + element.offsetHeight / 2;
> +    eventSender.mouseMoveTo(centerX, centerY);
> +}

This seems like a very useful function, maybe we could put it somewhere shared so other tests can use it too?

> LayoutTests/editing/pasteboard/datatransfer-items-drop-directoryReader.html:16
> +function moveMouseToCenterOfElement(element) {
> +    var centerX = element.offsetLeft + element.offsetWidth / 2;
> +    var centerY = element.offsetTop + element.offsetHeight / 2;
> +    eventSender.mouseMoveTo(centerX, centerY);
> +}

This seems like a very useful function, maybe we could put it somewhere shared so other tests can use it too?

[Chris Dumez]

Created attachment 319418 [details]
Patch

[Andreas Kling]

Comment on attachment 319418 [details]
Patch

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

> Source/WebCore/Modules/entriesapi/DOMFileSystem.cpp:159
> +    m_workQueue->dispatch([this, completionHandler = WTFMove(completionHandler), fullPath = fullPath.isolatedCopy(), directoryPtr = &directory]() mutable {
> +        auto listedChildren = listDirectoryWithMetadata(fullPath);
> +        callOnMainThread([this, completionHandler = WTFMove(completionHandler), listedChildren = crossThreadCopy(listedChildren), directoryPtr]() mutable {
> +            completionHandler(toFileSystemEntries(*this, WTFMove(listedChildren), directoryPtr->virtualPath()));
> +        });
> +    });

Since the directoryPtr is only used to retrieve the virtualPath() here, we can avoid the mysterious lifetime implications by simply capturing the virtual path instead of the directory.

> Source/WebCore/Modules/entriesapi/FileSystemDirectoryReader.cpp:39
> +    : ActiveDOMObject(&context)

The ActiveDOMObject constructor could probably take the ScriptExecutionContext by reference.

> Source/WebCore/Modules/entriesapi/FileSystemDirectoryReader.cpp:66
> +    if (m_isReading) {
> +        if (errorCallback)
> +            errorCallback->scheduleCallback(context, DOMException::create(Exception { InvalidStateError, ASCIILiteral("Directory reader is already reading") }));
> +        return;
> +    }

This case is not covered in the layout tests.

> Source/WebCore/Modules/entriesapi/FileSystemDirectoryReader.cpp:72
> +    if (m_error) {
> +        if (errorCallback)
> +            errorCallback->scheduleCallback(context, DOMException::create(*m_error));
> +        return;
> +    }

This is also not covered, although I don't know an easy way to trip an error condition here.

> Source/WebCore/dom/ExceptionOr.h:192
> +        return CrossThreadCopier<T>::copy(source.returnValue());

This could use crossThreadCopy :)

[Chris Dumez]

Created attachment 319432 [details]
Patch

[Chris Dumez]

Created attachment 319433 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 319433 [details]
Patch

Clearing flags on attachment: 319433

Committed r221414: <http://trac.webkit.org/changeset/221414>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 319385 [details]
Patch

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

>> Source/WebCore/Modules/entriesapi/DOMFileSystem.cpp:119
>> +    virtualPath.split('/', false /* allowEmpty */, components);
> 
> auto + overload of split() would look nice :)

You should use StringView::split, which makes a lot of separate StringView. No need to allocate all these strings just to build up a single string at the end. Do it all with StringView.