Bug 176091 - Implement FileSystemDirectoryReader.readEntries()
Summary: Implement FileSystemDirectoryReader.readEntries()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar, WebExposed
Depends on:
Blocks: 175976
  Show dependency treegraph
 
Reported: 2017-08-29 22:19 PDT by Chris Dumez
Modified: 2017-09-03 20:35 PDT (History)
15 users (show)

See Also:


Attachments
WIP Patch (34.54 KB, patch)
2017-08-29 22:21 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (33.78 KB, patch)
2017-08-29 22:28 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (34.31 KB, patch)
2017-08-30 09:22 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (35.62 KB, patch)
2017-08-30 11:27 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (42.70 KB, patch)
2017-08-30 12:04 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (47.86 KB, patch)
2017-08-30 16:01 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (53.09 KB, patch)
2017-08-30 19:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (53.15 KB, patch)
2017-08-30 19:18 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-08-29 22:19:19 PDT
Implement FileSystemDirectoryReader.readEntries():
- https://wicg.github.io/entries-api/#dom-filesystemdirectoryreader-readentries
Comment 1 Chris Dumez 2017-08-29 22:21:24 PDT
Created attachment 319342 [details]
WIP Patch
Comment 2 Build Bot 2017-08-29 22:22:45 PDT
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.
Comment 3 Chris Dumez 2017-08-29 22:28:05 PDT
Created attachment 319344 [details]
WIP Patch
Comment 4 Build Bot 2017-08-29 22:29:49 PDT
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.
Comment 5 Chris Dumez 2017-08-30 09:22:08 PDT
Created attachment 319369 [details]
WIP Patch
Comment 6 Chris Dumez 2017-08-30 11:27:35 PDT
Created attachment 319383 [details]
WIP Patch
Comment 7 Radar WebKit Bug Importer 2017-08-30 11:51:01 PDT
<rdar://problem/34168015>
Comment 8 Chris Dumez 2017-08-30 12:04:13 PDT
Created attachment 319385 [details]
Patch
Comment 9 Andreas Kling 2017-08-30 14:17:08 PDT
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?
Comment 10 Chris Dumez 2017-08-30 16:01:59 PDT
Created attachment 319418 [details]
Patch
Comment 11 Andreas Kling 2017-08-30 17:06:16 PDT
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 :)
Comment 12 Chris Dumez 2017-08-30 19:03:55 PDT
Created attachment 319432 [details]
Patch
Comment 13 Chris Dumez 2017-08-30 19:18:43 PDT
Created attachment 319433 [details]
Patch
Comment 14 WebKit Commit Bot 2017-08-30 20:48:25 PDT
Comment on attachment 319433 [details]
Patch

Clearing flags on attachment: 319433

Committed r221414: <http://trac.webkit.org/changeset/221414>
Comment 15 WebKit Commit Bot 2017-08-30 20:48:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Darin Adler 2017-09-03 20:35:36 PDT
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.