arc: Use directory cache when getting ExtraFileMetadata for an entry

The cache was introduced in https://crrev.com/c/616409 but was only used
to get File::Info for an entry, and the calls to get ExtraFileMetadata
made a mojo call each time, even though both functions draw data from
the same source (ARC Document objects). It did not impact read-only
providers, as the extra metadata returned for them is currently static,
but this will change when dynamic metadata is added (DocumentsProvider
thumbnail flag).

Also, ArcDocumentsProviderRoot::GetMetadata is renamed to
GetExtraFileMetadata to disambiguate it from a file system operation
with the same name.

Bug: 1049966
Change-Id: I94e0dc50e8a646a68b8c7b76e398c7777f4da121
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2464518
Commit-Queue: Alexander Bolodurin <alexbn@google.com>
Reviewed-by: Naoki Fukino <fukino@chromium.org>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816542}

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

@@ -117,16 +117,9 @@ void ArcDocumentsProviderRoot::GetFileInfo(const base::FilePath& path,
     return;
   }
 
-  base::FilePath basename = path.BaseName();
-  base::FilePath parent = path.DirName();
-  if (parent.value() == base::FilePath::kCurrentDirectory)
-    parent = base::FilePath();
-
-  ResolveToDocumentId(
-      parent,
-      base::BindOnce(&ArcDocumentsProviderRoot::GetFileInfoWithParentDocumentId,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(callback),
-                     basename));
+  GetDocument(path, base::BindOnce(
+                        &ArcDocumentsProviderRoot::GetFileInfoFromDocument,
+                        weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
 void ArcDocumentsProviderRoot::ReadDirectory(const base::FilePath& path,
@@ -269,8 +262,9 @@ void ArcDocumentsProviderRoot::ResolveToContentUrl(
                 weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
-void ArcDocumentsProviderRoot::GetMetadata(const base::FilePath& path,
-                                           GetMetadataCallback callback) {
+void ArcDocumentsProviderRoot::GetExtraFileMetadata(
+    const base::FilePath& path,
+    GetExtraMetadataCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   if (path.IsAbsolute()) {
@@ -283,10 +277,10 @@ void ArcDocumentsProviderRoot::GetMetadata(const base::FilePath& path,
     std::move(callback).Run(base::File::FILE_OK, {});
     return;
   }
-  ResolveToDocumentId(
-      path,
-      base::BindOnce(&ArcDocumentsProviderRoot::GetMetadataWithDocumentId,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+
+  GetDocument(path, base::BindOnce(
+                        &ArcDocumentsProviderRoot::GetExtraMetadataFromDocument,
+                        weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
 void ArcDocumentsProviderRoot::SetDirectoryCacheExpireSoonForTesting() {
@@ -301,44 +295,16 @@ void ArcDocumentsProviderRoot::OnWatchersCleared() {
     entry.second = kInvalidWatcherData;
 }
 
-void ArcDocumentsProviderRoot::GetFileInfoWithParentDocumentId(
+void ArcDocumentsProviderRoot::GetFileInfoFromDocument(
     GetFileInfoCallback callback,
-    const base::FilePath& basename,
-    const std::string& parent_document_id) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (parent_document_id.empty()) {
-    std::move(callback).Run(base::File::FILE_ERROR_NOT_FOUND,
-                            base::File::Info());
-    return;
-  }
-  ReadDirectoryInternal(
-      parent_document_id, false /* force_refresh */,
-      base::BindOnce(
-          &ArcDocumentsProviderRoot::GetFileInfoWithNameToDocumentMap,
-          weak_ptr_factory_.GetWeakPtr(), std::move(callback), basename));
-}
-
-void ArcDocumentsProviderRoot::GetFileInfoWithNameToDocumentMap(
-    GetFileInfoCallback callback,
-    const base::FilePath& basename,
     base::File::Error error,
-    const NameToDocumentMap& mapping) {
+    const mojom::DocumentPtr& document) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
-  if (error != base::File::FILE_OK) {
-    std::move(callback).Run(error, base::File::Info());
-    return;
-  }
-
-  auto iter = mapping.find(basename.value());
-  if (iter == mapping.end()) {
-    std::move(callback).Run(base::File::FILE_ERROR_NOT_FOUND,
-                            base::File::Info());
+  if (document.is_null()) {
+    std::move(callback).Run(base::File::FILE_ERROR_NOT_FOUND, {});
     return;
   }
 
-  const auto& document = iter->second;
-
   base::File::Info info;
   info.size = document->size;
   info.is_directory = document->mime_type == kAndroidDirectoryMimeType;
@@ -748,22 +714,10 @@ void ArcDocumentsProviderRoot::ResolveToContentUrlWithDocumentId(
   std::move(callback).Run(BuildDocumentUrl(authority_, document_id));
 }
 
-void ArcDocumentsProviderRoot::GetMetadataWithDocumentId(
-    GetMetadataCallback callback,
-    const std::string& document_id) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (document_id.empty()) {
-    std::move(callback).Run(base::File::FILE_ERROR_NOT_FOUND, {});
-    return;
-  }
-  runner_->GetDocument(
-      authority_, document_id,
-      base::BindOnce(&ArcDocumentsProviderRoot::OnMetadataGotten,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
-}
-
-void ArcDocumentsProviderRoot::OnMetadataGotten(GetMetadataCallback callback,
-                                                mojom::DocumentPtr document) {
+void ArcDocumentsProviderRoot::GetExtraMetadataFromDocument(
+    GetExtraMetadataCallback callback,
+    base::File::Error error,
+    const mojom::DocumentPtr& document) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (document.is_null()) {
     std::move(callback).Run(base::File::FILE_ERROR_NOT_FOUND, {});
@@ -776,6 +730,61 @@ void ArcDocumentsProviderRoot::OnMetadataGotten(GetMetadataCallback callback,
   std::move(callback).Run(base::File::FILE_OK, metadata);
 }
 
+void ArcDocumentsProviderRoot::GetDocument(const base::FilePath& path,
+                                           GetDocumentCallback callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  base::FilePath basename = path.BaseName();
+  base::FilePath parent = path.DirName();
+  if (parent.value() == base::FilePath::kCurrentDirectory)
+    parent = base::FilePath();
+
+  ResolveToDocumentId(
+      parent,
+      base::BindOnce(&ArcDocumentsProviderRoot::GetDocumentWithParentDocumentId,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback),
+                     basename));
+}
+
+void ArcDocumentsProviderRoot::GetDocumentWithParentDocumentId(
+    GetDocumentCallback callback,
+    const base::FilePath& basename,
+    const std::string& parent_document_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (parent_document_id.empty()) {
+    std::move(callback).Run(base::File::FILE_ERROR_NOT_FOUND,
+                            mojom::DocumentPtr());
+    return;
+  }
+  ReadDirectoryInternal(
+      parent_document_id, false /* force_refresh */,
+      base::BindOnce(
+          &ArcDocumentsProviderRoot::GetDocumentWithNameToDocumentMap,
+          weak_ptr_factory_.GetWeakPtr(), std::move(callback), basename));
+}
+
+void ArcDocumentsProviderRoot::GetDocumentWithNameToDocumentMap(
+    GetDocumentCallback callback,
+    const base::FilePath& basename,
+    base::File::Error error,
+    const NameToDocumentMap& mapping) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  if (error != base::File::FILE_OK) {
+    std::move(callback).Run(error, mojom::DocumentPtr());
+    return;
+  }
+
+  auto iter = mapping.find(basename.value());
+  if (iter == mapping.end()) {
+    std::move(callback).Run(base::File::FILE_ERROR_NOT_FOUND,
+                            mojom::DocumentPtr());
+    return;
+  }
+
+  std::move(callback).Run(base::File::FILE_OK, iter->second);
+}
+
 void ArcDocumentsProviderRoot::ResolveToDocumentId(
     const base::FilePath& path,
     ResolveToDocumentIdCallback callback) {

chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h

@@ -65,7 +65,7 @@ class ArcDocumentsProviderRoot : public ArcFileSystemOperationRunner::Observer {
   using WatcherStatusCallback = storage::WatcherManager::StatusCallback;
   using ResolveToContentUrlCallback =
       base::OnceCallback<void(const GURL& content_url)>;
-  using GetMetadataCallback =
+  using GetExtraMetadataCallback =
       base::OnceCallback<void(base::File::Error error,
                               const ExtraFileMetadata& metadata)>;
 
@@ -188,7 +188,8 @@ class ArcDocumentsProviderRoot : public ArcFileSystemOperationRunner::Observer {
   // Get extra metadata of the file at |path|.
   // The metadata is about capatility of write operations.
   // See ExtraFileMetadata for the supported capabilities.
-  void GetMetadata(const base::FilePath& path, GetMetadataCallback callback);
+  void GetExtraFileMetadata(const base::FilePath& path,
+                            GetExtraMetadataCallback callback);
 
   // Instructs to make directory caches expire "soon" after callbacks are
   // called, that is, when the message loop gets idle.
@@ -214,14 +215,13 @@ class ArcDocumentsProviderRoot : public ArcFileSystemOperationRunner::Observer {
   using ReadDirectoryInternalCallback =
       base::OnceCallback<void(base::File::Error error,
                               const NameToDocumentMap& mapping)>;
+  using GetDocumentCallback =
+      base::OnceCallback<void(base::File::Error error,
+                              const mojom::DocumentPtr& document)>;
 
-  void GetFileInfoWithParentDocumentId(GetFileInfoCallback callback,
-                                       const base::FilePath& basename,
-                                       const std::string& parent_document_id);
-  void GetFileInfoWithNameToDocumentMap(GetFileInfoCallback callback,
-                                        const base::FilePath& basename,
-                                        base::File::Error error,
-                                        const NameToDocumentMap& mapping);
+  void GetFileInfoFromDocument(GetFileInfoCallback callback,
+                               base::File::Error error,
+                               const mojom::DocumentPtr& document);
 
   void ReadDirectoryWithDocumentId(ReadDirectoryCallback callback,
                                    const std::string& document_id);
@@ -315,11 +315,20 @@ class ArcDocumentsProviderRoot : public ArcFileSystemOperationRunner::Observer {
   void ResolveToContentUrlWithDocumentId(ResolveToContentUrlCallback callback,
                                          const std::string& document_id);
 
-  void GetMetadataWithDocumentId(GetMetadataCallback callback,
-                                 const std::string& document_id);
-  void OnMetadataGotten(GetMetadataCallback callback,
-                        mojom::DocumentPtr document);
+  void GetExtraMetadataFromDocument(GetExtraMetadataCallback callback,
+                                    base::File::Error error,
+                                    const mojom::DocumentPtr& document);
 
+  // Queries for a single document at the given |path|, using a directory cache,
+  // if present.
+  void GetDocument(const base::FilePath& path, GetDocumentCallback callback);
+  void GetDocumentWithParentDocumentId(GetDocumentCallback callback,
+                                       const base::FilePath& basename,
+                                       const std::string& parent_document_id);
+  void GetDocumentWithNameToDocumentMap(GetDocumentCallback callback,
+                                        const base::FilePath& basename,
+                                        base::File::Error error,
+                                        const NameToDocumentMap& mapping);
   // Resolves |path| to a document ID. Failures are indicated by an empty
   // document ID.
   void ResolveToDocumentId(const base::FilePath& path,

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

@@ -1326,7 +1326,7 @@ TEST_F(ArcDocumentsProviderRootTest, ResolveToContentUrlDups) {
 
 TEST_F(ArcDocumentsProviderRootTest, GetMetadataNonDeletable) {
   base::RunLoop run_loop;
-  root_->GetMetadata(
+  root_->GetExtraFileMetadata(
       base::FilePath(FILE_PATH_LITERAL("dir/no-delete.jpg")),
       base::BindOnce(
           [](base::RunLoop* run_loop, base::File::Error error,
@@ -1343,7 +1343,7 @@ TEST_F(ArcDocumentsProviderRootTest, GetMetadataNonDeletable) {
 
 TEST_F(ArcDocumentsProviderRootTest, GetMetadataNonRenamable) {
   base::RunLoop run_loop;
-  root_->GetMetadata(
+  root_->GetExtraFileMetadata(
       base::FilePath(FILE_PATH_LITERAL("dir/no-rename.jpg")),
       base::BindOnce(
           [](base::RunLoop* run_loop, base::File::Error error,
@@ -1360,7 +1360,7 @@ TEST_F(ArcDocumentsProviderRootTest, GetMetadataNonRenamable) {
 
 TEST_F(ArcDocumentsProviderRootTest, GetMetadataReadOnlyDirectory) {
   base::RunLoop run_loop;
-  root_->GetMetadata(
+  root_->GetExtraFileMetadata(
       base::FilePath(FILE_PATH_LITERAL("ro-dir")),
       base::BindOnce(
           [](base::RunLoop* run_loop, base::File::Error error,
@@ -1377,7 +1377,7 @@ TEST_F(ArcDocumentsProviderRootTest, GetMetadataReadOnlyDirectory) {
 
 TEST_F(ArcDocumentsProviderRootTest, GetMetadataNonExist) {
   base::RunLoop run_loop;
-  root_->GetMetadata(
+  root_->GetExtraFileMetadata(
       base::FilePath(FILE_PATH_LITERAL("dir/no-exist-file")),
       base::BindOnce(
           [](base::RunLoop* run_loop, base::File::Error error,

chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc

@@ -274,14 +274,13 @@ class SingleEntryPropertiesGetterForDocumentsProvider {
       CompleteGetEntryProperties(base::File::FILE_ERROR_NOT_FOUND);
       return;
     }
-    root->GetMetadata(
-        path,
-        base::BindOnce(
-            &SingleEntryPropertiesGetterForDocumentsProvider::OnGetMetadata,
-            weak_ptr_factory_.GetWeakPtr()));
+    root->GetExtraFileMetadata(
+        path, base::BindOnce(&SingleEntryPropertiesGetterForDocumentsProvider::
+                                 OnGetExtraFileMetadata,
+                             weak_ptr_factory_.GetWeakPtr()));
   }
 
-  void OnGetMetadata(
+  void OnGetExtraFileMetadata(
       base::File::Error error,
       const arc::ArcDocumentsProviderRoot::ExtraFileMetadata& metadata) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);