v8binding: Refactors filesystem's DirectoryReader.

This patch is a kind of follow-up of https://crrev.com/844450 .
This patch makes DirectoryReader follow the same style as
other filesystem's callbacks.

Bug: 778580
Change-Id: I64c89d54dfa8a91306a3ea6b94789a1fcdde2476
Reviewed-on: https://chromium-review.googlesource.com/890800
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533624}

third_party/WebKit/Source/modules/filesystem/DOMFileSystemBase.cpp

@@ -451,7 +451,7 @@ void DOMFileSystemBase::GetDirectory(
 int DOMFileSystemBase::ReadDirectory(
     DirectoryReaderBase* reader,
     const String& path,
-    DirectoryReaderOnDidReadCallback* success_callback,
+    EntriesCallbacks::OnDidGetEntriesCallback* success_callback,
     ErrorCallbackBase* error_callback,
     SynchronousType synchronous_type) {
   if (!FileSystem()) {

third_party/WebKit/Source/modules/filesystem/DOMFileSystemBase.h

@@ -48,7 +48,6 @@ class WebFileSystem;
 namespace blink {
 
 class DirectoryReaderBase;
-class DirectoryReaderOnDidReadCallback;
 class EntryBase;
 class ExecutionContext;
 class File;
@@ -152,7 +151,7 @@ class MODULES_EXPORT DOMFileSystemBase : public ScriptWrappable {
                     SynchronousType = kAsynchronous);
   int ReadDirectory(DirectoryReaderBase*,
                     const String& path,
-                    DirectoryReaderOnDidReadCallback*,
+                    EntriesCallbacks::OnDidGetEntriesCallback*,
                     ErrorCallbackBase*,
                     SynchronousType = kAsynchronous);
   bool WaitForAdditionalResult(int callbacks_id);

third_party/WebKit/Source/modules/filesystem/DirectoryReader.cpp

@@ -40,28 +40,31 @@ namespace blink {
 
 namespace {
 
-void RunEntriesCallback(V8EntriesCallback* callback,
-                        HeapVector<Member<Entry>>* entries) {
+void RunEntriesCallback(V8EntriesCallback* callback, EntryHeapVector* entries) {
   callback->handleEvent(*entries);
 }
 
 }  // namespace
 
 class DirectoryReader::EntriesCallbackHelper final
-    : public DirectoryReaderOnDidReadCallback {
+    : public EntriesCallbacks::OnDidGetEntriesCallback {
  public:
-  explicit EntriesCallbackHelper(DirectoryReader* reader) : reader_(reader) {}
-
-  void OnDidReadDirectoryEntries(const EntryHeapVector& entries) override {
-    reader_->AddEntries(entries);
+  static EntriesCallbackHelper* Create(DirectoryReader* reader) {
+    return new EntriesCallbackHelper(reader);
   }
 
   void Trace(blink::Visitor* visitor) override {
     visitor->Trace(reader_);
-    DirectoryReaderOnDidReadCallback::Trace(visitor);
+    EntriesCallbacks::OnDidGetEntriesCallback::Trace(visitor);
+  }
+
+  void OnSuccess(EntryHeapVector* entries) override {
+    reader_->AddEntries(*entries);
   }
 
  private:
+  explicit EntriesCallbackHelper(DirectoryReader* reader) : reader_(reader) {}
+
   // FIXME: This Member keeps the reader alive until all of the readDirectory
   // results are received. crbug.com/350285
   Member<DirectoryReader> reader_;
@@ -69,7 +72,9 @@ class DirectoryReader::EntriesCallbackHelper final
 
 class DirectoryReader::ErrorCallbackHelper final : public ErrorCallbackBase {
  public:
-  explicit ErrorCallbackHelper(DirectoryReader* reader) : reader_(reader) {}
+  static ErrorCallbackHelper* Create(DirectoryReader* reader) {
+    return new ErrorCallbackHelper(reader);
+  }
 
   void Invoke(FileError::ErrorCode error) override { reader_->OnError(error); }
 
@@ -79,6 +84,8 @@ class DirectoryReader::ErrorCallbackHelper final : public ErrorCallbackBase {
   }
 
  private:
+  explicit ErrorCallbackHelper(DirectoryReader* reader) : reader_(reader) {}
+
   Member<DirectoryReader> reader_;
 };
 
@@ -86,15 +93,13 @@ DirectoryReader::DirectoryReader(DOMFileSystemBase* file_system,
                                  const String& full_path)
     : DirectoryReaderBase(file_system, full_path), is_reading_(false) {}
 
-DirectoryReader::~DirectoryReader() = default;
-
 void DirectoryReader::readEntries(V8EntriesCallback* entries_callback,
                                   V8ErrorCallback* error_callback) {
   if (!is_reading_) {
     is_reading_ = true;
     Filesystem()->ReadDirectory(this, full_path_,
-                                new EntriesCallbackHelper(this),
-                                new ErrorCallbackHelper(this));
+                                EntriesCallbackHelper::Create(this),
+                                ErrorCallbackHelper::Create(this));
   }
 
   if (error_) {
@@ -104,7 +109,7 @@ void DirectoryReader::readEntries(V8EntriesCallback* entries_callback,
   }
 
   if (entries_callback_) {
-    // Non-null m_entriesCallback means multiple readEntries() calls are made
+    // Non-null entries_callback_ means multiple readEntries() calls are made
     // concurrently. We don't allow doing it.
     Filesystem()->ReportError(ScriptErrorCallback::Wrap(error_callback),
                               FileError::kInvalidStateErr);
@@ -112,14 +117,11 @@ void DirectoryReader::readEntries(V8EntriesCallback* entries_callback,
   }
 
   if (!has_more_entries_ || !entries_.IsEmpty()) {
-    if (entries_callback) {
-      auto* entries = new HeapVector<Member<Entry>>(std::move(entries_));
-      DOMFileSystem::ScheduleCallback(
-          Filesystem()->GetExecutionContext(),
-          WTF::Bind(&RunEntriesCallback, WrapPersistent(entries_callback),
-                    WrapPersistent(entries)));
-    }
-    entries_.clear();
+    EntryHeapVector* entries = new EntryHeapVector(std::move(entries_));
+    DOMFileSystem::ScheduleCallback(
+        Filesystem()->GetExecutionContext(),
+        WTF::Bind(&RunEntriesCallback, WrapPersistent(entries_callback),
+                  WrapPersistent(entries)));
     return;
   }
 
@@ -130,8 +132,7 @@ void DirectoryReader::readEntries(V8EntriesCallback* entries_callback,
 void DirectoryReader::AddEntries(const EntryHeapVector& entries) {
   entries_.AppendVector(entries);
   error_callback_ = nullptr;
-  if (entries_callback_) {
-    V8EntriesCallback* entries_callback = entries_callback_.Release();
+  if (V8EntriesCallback* entries_callback = entries_callback_.Release()) {
     EntryHeapVector entries;
     entries.swap(entries_);
     entries_callback->handleEvent(entries);
@@ -141,8 +142,9 @@ void DirectoryReader::AddEntries(const EntryHeapVector& entries) {
 void DirectoryReader::OnError(FileError::ErrorCode error) {
   error_ = error;
   entries_callback_ = nullptr;
-  if (error_callback_)
-    error_callback_->handleEvent(FileError::CreateDOMException(error));
+  if (V8ErrorCallback* error_callback = error_callback_.Release()) {
+    error_callback->handleEvent(FileError::CreateDOMException(error_));
+  }
 }
 
 void DirectoryReader::Trace(blink::Visitor* visitor) {

third_party/WebKit/Source/modules/filesystem/DirectoryReader.h

@@ -50,7 +50,7 @@ class DirectoryReader : public DirectoryReaderBase {
     return new DirectoryReader(file_system, full_path);
   }
 
-  ~DirectoryReader() override;
+  ~DirectoryReader() override = default;
 
   void readEntries(V8EntriesCallback*, V8ErrorCallback* = nullptr);
 

third_party/WebKit/Source/modules/filesystem/DirectoryReaderBase.h

@@ -68,17 +68,6 @@ class DirectoryReaderBase : public ScriptWrappable {
   bool has_more_entries_;
 };
 
-class DirectoryReaderOnDidReadCallback
-    : public GarbageCollectedFinalized<DirectoryReaderOnDidReadCallback> {
- public:
-  virtual ~DirectoryReaderOnDidReadCallback() = default;
-  virtual void Trace(blink::Visitor* visitor) {}
-  virtual void OnDidReadDirectoryEntries(const EntryHeapVector&) = 0;
-
- protected:
-  DirectoryReaderOnDidReadCallback() = default;
-};
-
 }  // namespace blink
 
 #endif  // DirectoryReaderBase_h

third_party/WebKit/Source/modules/filesystem/DirectoryReaderSync.cpp

@@ -40,62 +40,70 @@
 namespace blink {
 
 class DirectoryReaderSync::EntriesCallbackHelper final
-    : public DirectoryReaderOnDidReadCallback {
+    : public EntriesCallbacks::OnDidGetEntriesCallback {
  public:
-  explicit EntriesCallbackHelper(DirectoryReaderSync* reader)
-      : reader_(reader) {}
-
-  void OnDidReadDirectoryEntries(const EntryHeapVector& entries) override {
-    EntrySyncHeapVector sync_entries;
-    sync_entries.ReserveInitialCapacity(entries.size());
-    for (size_t i = 0; i < entries.size(); ++i)
-      sync_entries.UncheckedAppend(EntrySync::Create(entries[i].Get()));
-    reader_->AddEntries(sync_entries);
+  static EntriesCallbackHelper* Create(DirectoryReaderSync* reader) {
+    return new EntriesCallbackHelper(reader);
   }
 
   void Trace(blink::Visitor* visitor) override {
     visitor->Trace(reader_);
-    DirectoryReaderOnDidReadCallback::Trace(visitor);
+    EntriesCallbacks::OnDidGetEntriesCallback::Trace(visitor);
+  }
+
+  void OnSuccess(EntryHeapVector* entries) override {
+    reader_->entries_.ReserveCapacity(reader_->entries_.size() +
+                                      entries->size());
+    for (const auto& entry : *entries) {
+      reader_->entries_.UncheckedAppend(EntrySync::Create(entry.Get()));
+    }
   }
 
  private:
+  explicit EntriesCallbackHelper(DirectoryReaderSync* reader)
+      : reader_(reader) {}
+
   Member<DirectoryReaderSync> reader_;
 };
 
 class DirectoryReaderSync::ErrorCallbackHelper final
     : public ErrorCallbackBase {
  public:
-  explicit ErrorCallbackHelper(DirectoryReaderSync* reader) : reader_(reader) {}
-
-  void Invoke(FileError::ErrorCode error) override { reader_->SetError(error); }
+  static ErrorCallbackHelper* Create(DirectoryReaderSync* reader) {
+    return new ErrorCallbackHelper(reader);
+  }
 
   void Trace(blink::Visitor* visitor) override {
     visitor->Trace(reader_);
     ErrorCallbackBase::Trace(visitor);
   }
 
+  void Invoke(FileError::ErrorCode error) override {
+    reader_->error_code_ = error;
+  }
+
  private:
+  explicit ErrorCallbackHelper(DirectoryReaderSync* reader) : reader_(reader) {}
+
   Member<DirectoryReaderSync> reader_;
 };
 
 DirectoryReaderSync::DirectoryReaderSync(DOMFileSystemBase* file_system,
                                          const String& full_path)
-    : DirectoryReaderBase(file_system, full_path),
-      callbacks_id_(0),
-      error_code_(FileError::kOK) {}
-
-DirectoryReaderSync::~DirectoryReaderSync() = default;
+    : DirectoryReaderBase(file_system, full_path) {}
 
 EntrySyncHeapVector DirectoryReaderSync::readEntries(
     ExceptionState& exception_state) {
   if (!callbacks_id_) {
     callbacks_id_ = Filesystem()->ReadDirectory(
-        this, full_path_, new EntriesCallbackHelper(this),
-        new ErrorCallbackHelper(this), DOMFileSystemBase::kSynchronous);
+        this, full_path_, EntriesCallbackHelper::Create(this),
+        ErrorCallbackHelper::Create(this), DOMFileSystemBase::kSynchronous);
   }
 
-  if (error_code_ == FileError::kOK && has_more_entries_ && entries_.IsEmpty())
-    file_system_->WaitForAdditionalResult(callbacks_id_);
+  if (error_code_ == FileError::kOK && has_more_entries_ &&
+      entries_.IsEmpty()) {
+    CHECK(Filesystem()->WaitForAdditionalResult(callbacks_id_));
+  }
 
   if (error_code_ != FileError::kOK) {
     FileError::ThrowDOMException(exception_state, error_code_);

third_party/WebKit/Source/modules/filesystem/DirectoryReaderSync.h

@@ -52,16 +52,10 @@ class DirectoryReaderSync : public DirectoryReaderBase {
     return new DirectoryReaderSync(file_system, full_path);
   }
 
-  ~DirectoryReaderSync() override;
+  ~DirectoryReaderSync() override = default;
 
   EntrySyncHeapVector readEntries(ExceptionState&);
 
-  void AddEntries(const EntrySyncHeapVector& entries) {
-    entries_.AppendVector(entries);
-  }
-
-  void SetError(FileError::ErrorCode code) { error_code_ = code; }
-
   void Trace(blink::Visitor*) override;
 
  private:
@@ -70,9 +64,9 @@ class DirectoryReaderSync : public DirectoryReaderBase {
 
   DirectoryReaderSync(DOMFileSystemBase*, const String& full_path);
 
-  int callbacks_id_;
+  int callbacks_id_ = 0;
   EntrySyncHeapVector entries_;
-  FileError::ErrorCode error_code_;
+  FileError::ErrorCode error_code_ = FileError::kOK;
 };
 
 }  // namespace blink

third_party/WebKit/Source/modules/filesystem/FileSystemCallbacks.cpp

@@ -88,38 +88,6 @@ bool FileSystemCallbacksBase::ShouldScheduleCallback() const {
          execution_context_->IsContextPaused();
 }
 
-template <typename CB, typename CBArg>
-void FileSystemCallbacksBase::HandleEventOrScheduleCallback(CB* callback,
-                                                            CBArg* arg) {
-  DCHECK(callback);
-  if (callback) {
-    if (ShouldScheduleCallback()) {
-      DOMFileSystem::ScheduleCallback(
-          execution_context_.Get(),
-          WTF::Bind(&CB::handleEvent, WrapPersistent(callback),
-                    WrapPersistent(arg)));
-    } else {
-      callback->handleEvent(arg);
-    }
-  }
-  execution_context_.Clear();
-}
-
-template <typename CB>
-void FileSystemCallbacksBase::HandleEventOrScheduleCallback(CB* callback) {
-  DCHECK(callback);
-  if (callback) {
-    if (ShouldScheduleCallback()) {
-      DOMFileSystem::ScheduleCallback(
-          execution_context_.Get(),
-          WTF::Bind(&CB::handleEvent, WrapPersistent(callback)));
-    } else {
-      callback->handleEvent();
-    }
-  }
-  execution_context_.Clear();
-}
-
 template <typename CallbackMemberFunction,
           typename CallbackClass,
           typename... Args>
@@ -213,7 +181,7 @@ void EntryCallbacks::DidSucceed() {
 // EntriesCallbacks -----------------------------------------------------------
 
 std::unique_ptr<AsyncFileSystemCallbacks> EntriesCallbacks::Create(
-    DirectoryReaderOnDidReadCallback* success_callback,
+    OnDidGetEntriesCallback* success_callback,
     ErrorCallbackBase* error_callback,
     ExecutionContext* context,
     DirectoryReaderBase* directory_reader,
@@ -222,12 +190,11 @@ std::unique_ptr<AsyncFileSystemCallbacks> EntriesCallbacks::Create(
       success_callback, error_callback, context, directory_reader, base_path));
 }
 
-EntriesCallbacks::EntriesCallbacks(
-    DirectoryReaderOnDidReadCallback* success_callback,
-    ErrorCallbackBase* error_callback,
-    ExecutionContext* context,
-    DirectoryReaderBase* directory_reader,
-    const String& base_path)
+EntriesCallbacks::EntriesCallbacks(OnDidGetEntriesCallback* success_callback,
+                                   ErrorCallbackBase* error_callback,
+                                   ExecutionContext* context,
+                                   DirectoryReaderBase* directory_reader,
+                                   const String& base_path)
     : FileSystemCallbacksBase(error_callback,
                               directory_reader->Filesystem(),
                               context),
@@ -239,23 +206,24 @@ EntriesCallbacks::EntriesCallbacks(
 
 void EntriesCallbacks::DidReadDirectoryEntry(const String& name,
                                              bool is_directory) {
-  if (is_directory)
-    entries_.push_back(
-        DirectoryEntry::Create(directory_reader_->Filesystem(),
-                               DOMFilePath::Append(base_path_, name)));
-  else
-    entries_.push_back(
-        FileEntry::Create(directory_reader_->Filesystem(),
-                          DOMFilePath::Append(base_path_, name)));
+  DOMFileSystemBase* filesystem = directory_reader_->Filesystem();
+  const String& path = DOMFilePath::Append(base_path_, name);
+  Entry* entry =
+      is_directory
+          ? static_cast<Entry*>(DirectoryEntry::Create(filesystem, path))
+          : static_cast<Entry*>(FileEntry::Create(filesystem, path));
+  entries_.push_back(entry);
 }
 
 void EntriesCallbacks::DidReadDirectoryEntries(bool has_more) {
   directory_reader_->SetHasMoreEntries(has_more);
-  EntryHeapVector entries;
-  entries.swap(entries_);
-  // FIXME: delay the callback iff shouldScheduleCallback() is true.
-  if (success_callback_)
-    success_callback_->OnDidReadDirectoryEntries(entries);
+  EntryHeapVector* entries = new EntryHeapVector(std::move(entries_));
+
+  if (!success_callback_)
+    return;
+
+  InvokeOrScheduleCallback(&OnDidGetEntriesCallback::OnSuccess,
+                           success_callback_.Get(), entries);
 }
 
 // FileSystemCallbacks --------------------------------------------------------

third_party/WebKit/Source/modules/filesystem/FileSystemCallbacks.h

@@ -34,6 +34,7 @@
 #include <memory>
 
 #include "core/fileapi/FileError.h"
+#include "modules/filesystem/EntryHeapVector.h"
 #include "platform/AsyncFileSystemCallbacks.h"
 #include "platform/FileSystemType.h"
 #include "platform/heap/Handle.h"
@@ -45,7 +46,6 @@ namespace blink {
 class DOMFileSystem;
 class DOMFileSystemBase;
 class DirectoryReaderBase;
-class DirectoryReaderOnDidReadCallback;
 class Entry;
 class ExecutionContext;
 class File;
@@ -86,12 +86,6 @@ class FileSystemCallbacksBase : public AsyncFileSystemCallbacks {
 
   bool ShouldScheduleCallback() const;
 
-  template <typename CB, typename CBArg>
-  void HandleEventOrScheduleCallback(CB*, CBArg*);
-
-  template <typename CB>
-  void HandleEventOrScheduleCallback(CB*);
-
   // Invokes the given callback synchronously or asynchronously depending on
   // the result of |ShouldScheduleCallback|.
   template <typename CallbackMemberFunction,
@@ -173,8 +167,19 @@ class EntryCallbacks final : public FileSystemCallbacksBase {
 
 class EntriesCallbacks final : public FileSystemCallbacksBase {
  public:
+  class OnDidGetEntriesCallback
+      : public GarbageCollectedFinalized<OnDidGetEntriesCallback> {
+   public:
+    virtual ~OnDidGetEntriesCallback() = default;
+    virtual void Trace(blink::Visitor*) {}
+    virtual void OnSuccess(EntryHeapVector*) = 0;
+
+   protected:
+    OnDidGetEntriesCallback() = default;
+  };
+
   static std::unique_ptr<AsyncFileSystemCallbacks> Create(
-      DirectoryReaderOnDidReadCallback*,
+      OnDidGetEntriesCallback*,
       ErrorCallbackBase*,
       ExecutionContext*,
       DirectoryReaderBase*,
@@ -183,12 +188,12 @@ class EntriesCallbacks final : public FileSystemCallbacksBase {
   void DidReadDirectoryEntries(bool has_more) override;
 
  private:
-  EntriesCallbacks(DirectoryReaderOnDidReadCallback*,
+  EntriesCallbacks(OnDidGetEntriesCallback*,
                    ErrorCallbackBase*,
                    ExecutionContext*,
                    DirectoryReaderBase*,
                    const String& base_path);
-  Persistent<DirectoryReaderOnDidReadCallback> success_callback_;
+  Persistent<OnDidGetEntriesCallback> success_callback_;
   Persistent<DirectoryReaderBase> directory_reader_;
   String base_path_;
   PersistentHeapVector<Member<Entry>> entries_;

third_party/WebKit/Source/modules/filesystem/SyncCallbackHelper.h

@@ -115,6 +115,7 @@ class DOMFileSystemCallbacksSyncHelper final
 using EntryCallbacksSyncHelper =
     DOMFileSystemCallbacksSyncHelper<EntryCallbacks::OnDidGetEntryCallback,
                                      Entry>;
+
 using FileSystemCallbacksSyncHelper = DOMFileSystemCallbacksSyncHelper<
     FileSystemCallbacks::OnDidOpenFileSystemCallback,
     DOMFileSystem>;