mediaview: Introduce ArcDocumentsProviderRoot.

chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc

Index: chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc
diff --git a/chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc b/chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc
index d14a96ac86b43c395a1063f0562ab92a9c412e50..dda144dc4512e6d92ec327e1053fbada569f6998 100644
--- a/chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc
+++ b/chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc
@@ -4,19 +4,28 @@
 
 #include "chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.h"
 
+#include <utility>
+
+#include "base/bind.h"
 #include "base/callback.h"
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
+#include "chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h"
+#include "chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_map.h"
+#include "chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h"
 #include "content/public/browser/browser_thread.h"
 #include "storage/browser/blob/shareable_file_reference.h"
+#include "storage/browser/fileapi/file_system_operation_context.h"
+#include "storage/browser/fileapi/file_system_url.h"
 
 using content::BrowserThread;
 
 namespace arc {
 
-ArcDocumentsProviderAsyncFileUtil::ArcDocumentsProviderAsyncFileUtil() =
-    default;
+ArcDocumentsProviderAsyncFileUtil::ArcDocumentsProviderAsyncFileUtil(
+    ArcDocumentsProviderRootMap* roots)
+    : roots_(roots), weak_ptr_factory_(this) {}
 
 ArcDocumentsProviderAsyncFileUtil::~ArcDocumentsProviderAsyncFileUtil() =
     default;
@@ -59,8 +68,22 @@ void ArcDocumentsProviderAsyncFileUtil::GetFileInfo(
     int fields,
     const GetFileInfoCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  NOTIMPLEMENTED();  // TODO(crbug.com/671511): Implement this function.
-  callback.Run(base::File::FILE_ERROR_NOT_FOUND, base::File::Info());
+  DCHECK_EQ(storage::kFileSystemTypeArcDocumentsProvider, url.type());
+
+  base::FilePath path;
+  ArcDocumentsProviderRoot* root = roots_->ParseAndLookup(url, &path);
+  if (!root) {
+    callback.Run(base::File::FILE_ERROR_NOT_FOUND, base::File::Info());
+    return;
+  }
+
+  root->GetFileInfo(
+      path,
+      base::Bind(&ArcDocumentsProviderAsyncFileUtil::OnGetFileInfo,
+                 // It is guaranteed that |this| outlives |context|, so keep

[hashimoto, 2016/12/15 03:53:22]

I don't understand why this becomes the reason to keep the context object alive.
Why do you need to do this?

[Shuhei Takahashi, 2016/12/15 05:21:53]

From AsyncFileUtil comment:

 // As far as an instance of this class is owned by a FileSystemBackend
 // (which is owned by FileSystemContext), it's guaranteed that this instance's
 // alive while FileSystemOperationContext given to each operation is kept
 // alive. (Note that this instance might be freed on different thread
 // from the thread it is created.)

https://cs.chromium.org/chromium/src/storage/browser/fileapi/async_file_util....


So the comment guarantees that, as long as we keep |context| alive, |this| is
also kept alive. Other than that it says nothing; in particular, about canceling
operations. So, assuming this comment is the only specification we can rely on,
it is *wrong* to use WeakPtr here because we are not explicitly allowed to
cancel calling |callback| after |this| is deleted, and the only way to do the
business correctly is to keep |context| (and |this|) alive until the operation
finishes and invoke |callback| all the time.

If you know any other explicit contract we can rely on, please let me know.

[Shuhei Takahashi, 2016/12/15 05:26:15]

Oh, BTW, you're right that weak_ptr_factory_ is useless. I removed it.

[hashimoto, 2016/12/15 10:34:57]

Sorry, I still don't understand why the said comment becomes the reason to keep
FileSystemOperationContext alive.
Existing implementations don't keep FileSystemOperationContext alive if
unnecessary.

The said comment was added in https://chromiumcodereview.appspot.com/16413007.
What it says is just the fact that AsyncFileUtil doesn't die as long as
FileSystemOperationContext is alive because FileSystemOperationContext has a
scoped_refptr of FileSystemContext which owns FileSystemBackend.
In Chrome, when destroying an object, usually you shouldn't expect the callback
to get called for any in-flight operations.

+                 // |context| until the operation finishes.
+                 base::Unretained(this), base::Passed(std::move(context)),

[hashimoto, 2016/12/15 03:53:22]

Why don't you use the newly added WeakPtrFactory here?

[Shuhei Takahashi, 2016/12/15 05:21:53]

Ditto.

+                 callback));
 }
 
 void ArcDocumentsProviderAsyncFileUtil::ReadDirectory(
@@ -68,9 +91,27 @@ void ArcDocumentsProviderAsyncFileUtil::ReadDirectory(
     const storage::FileSystemURL& url,
     const ReadDirectoryCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  NOTIMPLEMENTED();  // TODO(crbug.com/671511): Implement this function.
-  callback.Run(base::File::FILE_ERROR_NOT_FOUND, EntryList(),
-               false /* has_more */);
+  DCHECK_EQ(storage::kFileSystemTypeArcDocumentsProvider, url.type());
+
+  base::FilePath path;
+  ArcDocumentsProviderRoot* root = roots_->ParseAndLookup(url, &path);
+  if (!root) {
+    callback.Run(base::File::FILE_ERROR_NOT_FOUND, EntryList(), false);
+    return;
+  }
+
+  root->ReadDirectory(
+      path,
+      base::Bind(
+          &ArcDocumentsProviderAsyncFileUtil::OnReadDirectory,
+          // It is guaranteed that |this| outlives |context|, so keep

[hashimoto, 2016/12/15 03:53:21]

ditto.

[Shuhei Takahashi, 2016/12/15 05:21:53]

Ditto.

+          // |context| until the operation finishes.
+          // In this case we can't use base::Passed() because the callback
+          // might be called multiple times with |has_more| = true.
+          base::Unretained(this),

[hashimoto, 2016/12/15 03:53:22]

ditto.

[Shuhei Takahashi, 2016/12/15 05:21:53]

Ditto.

+          base::Owned(new std::unique_ptr<storage::FileSystemOperationContext>(
+              std::move(context))),
+          callback));
 }
 
 void ArcDocumentsProviderAsyncFileUtil::Touch(
@@ -165,4 +206,28 @@ void ArcDocumentsProviderAsyncFileUtil::CreateSnapshotFile(
                scoped_refptr<storage::ShareableFileReference>());
 }
 
+void ArcDocumentsProviderAsyncFileUtil::OnGetFileInfo(
+    std::unique_ptr<storage::FileSystemOperationContext> context,
+    const GetFileInfoCallback& callback,
+    base::File::Error result,
+    const base::File::Info& file_info) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  callback.Run(result, file_info);
+}
+
+void ArcDocumentsProviderAsyncFileUtil::OnReadDirectory(
+    std::unique_ptr<storage::FileSystemOperationContext>* context,
+    const ReadDirectoryCallback& callback,
+    base::File::Error result,
+    const EntryList& file_list,
+    bool has_more) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  callback.Run(result, file_list, has_more);
+  // |has_more| = false indicates the last call, so release |context|.
+  if (!has_more) {
+    DCHECK(*context);
+    context->reset();
+  }
+}
+
 }  // namespace arc