Implement FileSystemDirectoryReader.readEntries(): - https://wicg.github.io/entries-api/#dom-filesystemdirectoryreader-readentries
Created attachment 319342 [details] WIP Patch
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.
Created attachment 319344 [details] WIP Patch
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.
Created attachment 319369 [details] WIP Patch
Created attachment 319383 [details] WIP Patch
<rdar://problem/34168015>
Created attachment 319385 [details] Patch
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?
Created attachment 319418 [details] Patch
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 :)
Created attachment 319432 [details] Patch
Created attachment 319433 [details] Patch
Comment on attachment 319433 [details] Patch Clearing flags on attachment: 319433 Committed r221414: <http://trac.webkit.org/changeset/221414>
All reviewed patches have been landed. Closing bug.
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.