Update path_util::ConvertToContentUrl to handling multiple URLs at a time.

Background:
* We are migrating callers of ConvertPathToArcUrl (sync) to using ConvertToContentUrl (async) instead.
* exo_parts.cc (a caller of ConvertToContentUrl) needs to handle multiple URLs at a time,
  so it converts single-URL callbacks to multiple-URLs callback using BarrierClosure.
* It turns out arc_file_tasks.cc (a caller of ConvertPathToArcUrl) also needs to handle multiple URLs at a time.
* Moving the BarrierClosure logic from exo_parts to path_util, so we don't need to duplicate the same BarrierClosure logic in arc_file_tasks after migration.

Bug: chromium:811679
Test: unit_test
Change-Id: I461651bac3e5110afdd7b97d39576436cb3a92fd
Reviewed-on: https://chromium-review.googlesource.com/983335Reviewed-by: David Reveman <reveman@chromium.org>
Reviewed-by: Daichi Hirono <hirono@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>
Commit-Queue: Satoshi Niwa <niwa@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547406}

chrome/browser/chromeos/file_manager/path_util.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/chromeos/file_manager/path_util.h"
 
+#include "base/barrier_closure.h"
 #include "base/logging.h"
 #include "base/sys_info.h"
 #include "chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h"
@@ -47,9 +48,19 @@ Profile* GetPrimaryProfile() {
   return chromeos::ProfileHelper::Get()->GetProfileByUser(primary_user);
 }
 
-void OnContentUrlResolved(ConvertToContentUrlCallback callback,
+// Helper function for |ConvertToContentUrls|.
+void OnSingleContentUrlResolved(const base::RepeatingClosure& barrier_closure,
+                                std::vector<GURL>* out_urls,
+                                size_t index,
                                 const GURL& url) {
-  std::move(callback).Run(url);
+  (*out_urls)[index] = url;
+  barrier_closure.Run();
+}
+
+// Helper function for |ConvertToContentUrls|.
+void OnAllContentUrlsResolved(ConvertToContentUrlsCallback callback,
+                              std::unique_ptr<std::vector<GURL>> urls) {
+  std::move(callback).Run(*urls);
 }
 
 }  // namespace
@@ -143,42 +154,57 @@ bool ConvertPathToArcUrl(const base::FilePath& path, GURL* arc_url_out) {
   return false;
 }
 
-void ConvertToContentUrl(const storage::FileSystemURL& file_system_url,
-                         ConvertToContentUrlCallback callback) {
+void ConvertToContentUrls(
+    const std::vector<storage::FileSystemURL>& file_system_urls,
+    ConvertToContentUrlsCallback callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  GURL arc_url;
-  if (file_system_url.mount_type() == storage::kFileSystemTypeExternal &&
-      ConvertPathToArcUrl(file_system_url.path(), &arc_url)) {
-    std::move(callback).Run(arc_url);
+  if (file_system_urls.empty()) {
+    std::move(callback).Run(std::vector<GURL>());
     return;
   }
 
-  Profile* primary_profile = GetPrimaryProfile();
-  if (!primary_profile) {
-    std::move(callback).Run(GURL());
-    return;
+  Profile* profile = GetPrimaryProfile();
+  auto* documents_provider_root_map =
+      profile ? arc::ArcDocumentsProviderRootMap::GetForBrowserContext(profile)
+              : nullptr;
+
+  // To keep the original order, prefill |out_urls| with empty URLs and
+  // specify index when updating it like (*out_urls)[index] = url.
+  auto out_urls = std::make_unique<std::vector<GURL>>(file_system_urls.size());
+  auto* out_urls_ptr = out_urls.get();
+  auto barrier = base::BarrierClosure(
+      file_system_urls.size(),
+      base::BindOnce(&OnAllContentUrlsResolved, std::move(callback),
+                     std::move(out_urls)));
+  auto single_content_url_callback =
+      base::BindRepeating(&OnSingleContentUrlResolved, barrier, out_urls_ptr);
+
+  for (size_t index = 0; index < file_system_urls.size(); ++index) {
+    const auto& file_system_url = file_system_urls[index];
+    GURL arc_url;
+    if (file_system_url.mount_type() == storage::kFileSystemTypeExternal &&
+        ConvertPathToArcUrl(file_system_url.path(), &arc_url)) {
+      single_content_url_callback.Run(index, arc_url);
+      continue;
     }
 
-  auto* documents_provider_root_map =
-      arc::ArcDocumentsProviderRootMap::GetForBrowserContext(primary_profile);
     if (!documents_provider_root_map) {
-    std::move(callback).Run(GURL());
-    return;
+      single_content_url_callback.Run(index, GURL());
+      continue;
     }
 
-  base::FilePath file_path;
+    base::FilePath filepath;
     auto* documents_provider_root =
-      documents_provider_root_map->ParseAndLookup(file_system_url, &file_path);
+        documents_provider_root_map->ParseAndLookup(file_system_url, &filepath);
     if (!documents_provider_root) {
-    std::move(callback).Run(GURL());
-    return;
+      single_content_url_callback.Run(index, GURL());
+      continue;
     }
 
-  // TODO(niwa): Remove OnContentUrlResolved once ResolveToContentUrl is
-  //             migrated from base::Callback to base::OnceCallback.
     documents_provider_root->ResolveToContentUrl(
-      file_path, base::Bind(&OnContentUrlResolved, base::Passed(&callback)));
+        filepath, base::BindRepeating(single_content_url_callback, index));
+  }
 }
 
 }  // namespace util

chrome/browser/chromeos/file_manager/path_util.h

@@ -43,20 +43,22 @@ bool MigratePathFromOldFormat(Profile* profile,
 // The canonical mount point name for "Downloads" folder.
 std::string GetDownloadsMountPointName(Profile* profile);
 
-// DEPRECATED. Use |ConvertToContentUrl| instead.
+// DEPRECATED. Use |ConvertToContentUrls| instead.
 // While this function can convert paths under Downloads, /media/removable
 // and /special/drive, this CANNOT convert paths under ARC media directories
 // (/special/arc-documents-provider).
 // TODO(crbug.com/811679): Migrate all callers and remove this.
 bool ConvertPathToArcUrl(const base::FilePath& path, GURL* arc_url_out);
 
-using ConvertToContentUrlCallback =
-    base::OnceCallback<void(const GURL& content_url)>;
+using ConvertToContentUrlsCallback =
+    base::OnceCallback<void(const std::vector<GURL>& content_urls)>;
 
-// Asynchronously converts a Chrome OS file system URL to a content:// URL.
-// Returns an empty GURL if |file_system_url| is not convertible.
-void ConvertToContentUrl(const storage::FileSystemURL& file_system_url,
-                         ConvertToContentUrlCallback callback);
+// Asynchronously converts Chrome OS file system URLs to content:// URLs.
+// Always returns a vector of the same size as |file_system_urls|.
+// Empty GURLs are filled in the vector if conversion fails.
+void ConvertToContentUrls(
+    const std::vector<storage::FileSystemURL>& file_system_urls,
+    ConvertToContentUrlsCallback callback);
 
 }  // namespace util
 }  // namespace file_manager

chrome/browser/chromeos/file_manager/path_util_unittest.cc

@@ -27,6 +27,8 @@
 #include "storage/browser/fileapi/external_mount_points.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using storage::FileSystemURL;
+
 namespace file_manager {
 namespace util {
 namespace {
@@ -149,9 +151,9 @@ class FileManagerPathUtilConvertUrlTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(FileManagerPathUtilConvertUrlTest);
 };
 
-storage::FileSystemURL CreateExternalURL(const base::FilePath& path) {
-  return storage::FileSystemURL::CreateForTest(
-      GURL(), storage::kFileSystemTypeExternal, path);
+FileSystemURL CreateExternalURL(const base::FilePath& path) {
+  return FileSystemURL::CreateForTest(GURL(), storage::kFileSystemTypeExternal,
+                                      path);
 }
 
 TEST_F(FileManagerPathUtilConvertUrlTest, ConvertPathToArcUrl_Removable) {
@@ -201,105 +203,116 @@ TEST_F(FileManagerPathUtilConvertUrlTest, ConvertPathToArcUrl_Special) {
 }
 
 TEST_F(FileManagerPathUtilConvertUrlTest,
-       ConvertToContentUrl_InvalidMountType) {
+       ConvertToContentUrls_InvalidMountType) {
   base::RunLoop run_loop;
-  ConvertToContentUrl(
-      storage::FileSystemURL::CreateForTest(
+  ConvertToContentUrls(
+      std::vector<FileSystemURL>{FileSystemURL::CreateForTest(
           GURL(), storage::kFileSystemTypeTest,
-          base::FilePath::FromUTF8Unsafe("/media/removable/a/b/c")),
+          base::FilePath::FromUTF8Unsafe("/media/removable/a/b/c"))},
       base::BindOnce(
-          [](base::RunLoop* run_loop, const GURL& url) {
+          [](base::RunLoop* run_loop, const std::vector<GURL>& urls) {
             run_loop->Quit();
-            EXPECT_EQ(GURL(), url);
+            ASSERT_EQ(1U, urls.size());
+            EXPECT_EQ(GURL(), urls[0]);
           },
           &run_loop));
   run_loop.Run();
 }
 
-TEST_F(FileManagerPathUtilConvertUrlTest, ConvertToContentUrl_Removable) {
+TEST_F(FileManagerPathUtilConvertUrlTest, ConvertToContentUrls_Removable) {
   base::RunLoop run_loop;
-  ConvertToContentUrl(
-      CreateExternalURL(
-          base::FilePath::FromUTF8Unsafe("/media/removable/a/b/c")),
+  ConvertToContentUrls(
+      std::vector<FileSystemURL>{CreateExternalURL(
+          base::FilePath::FromUTF8Unsafe("/media/removable/a/b/c"))},
       base::BindOnce(
-          [](base::RunLoop* run_loop, const GURL& url) {
+          [](base::RunLoop* run_loop, const std::vector<GURL>& urls) {
             run_loop->Quit();
+            ASSERT_EQ(1U, urls.size());
             EXPECT_EQ(
                 GURL("content://org.chromium.arc.removablemediaprovider/a/b/c"),
-                url);
+                urls[0]);
           },
           &run_loop));
   run_loop.Run();
 }
 
 TEST_F(FileManagerPathUtilConvertUrlTest,
-       ConvertToContentUrl_InvalidRemovable) {
+       ConvertToContentUrls_InvalidRemovable) {
   base::RunLoop run_loop;
-  ConvertToContentUrl(CreateExternalURL(base::FilePath::FromUTF8Unsafe(
-                          "/media/removable_foobar")),
+  ConvertToContentUrls(
+      std::vector<FileSystemURL>{CreateExternalURL(
+          base::FilePath::FromUTF8Unsafe("/media/removable_foobar"))},
       base::BindOnce(
-                          [](base::RunLoop* run_loop, const GURL& url) {
+          [](base::RunLoop* run_loop, const std::vector<GURL>& urls) {
             run_loop->Quit();
-                            EXPECT_EQ(GURL(), url);
+            ASSERT_EQ(1U, urls.size());
+            EXPECT_EQ(GURL(), urls[0]);
           },
           &run_loop));
   run_loop.Run();
 }
 
-TEST_F(FileManagerPathUtilConvertUrlTest, ConvertToContentUrl_Downloads) {
+TEST_F(FileManagerPathUtilConvertUrlTest, ConvertToContentUrls_Downloads) {
   const base::FilePath downloads = GetDownloadsFolderForProfile(
       chromeos::ProfileHelper::Get()->GetProfileByUserIdHashForTest(
           "user@gmail.com-hash"));
   base::RunLoop run_loop;
-  ConvertToContentUrl(
-      CreateExternalURL(downloads.AppendASCII("a/b/c")),
+  ConvertToContentUrls(
+      std::vector<FileSystemURL>{
+          CreateExternalURL(downloads.AppendASCII("a/b/c"))},
       base::BindOnce(
-          [](base::RunLoop* run_loop, const GURL& url) {
+          [](base::RunLoop* run_loop, const std::vector<GURL>& urls) {
             run_loop->Quit();
+            ASSERT_EQ(1U, urls.size());
             EXPECT_EQ(
                 GURL("content://org.chromium.arc.intent_helper.fileprovider/"
                      "download/a/b/c"),
-                url);
+                urls[0]);
           },
           &run_loop));
   run_loop.Run();
 }
 
 TEST_F(FileManagerPathUtilConvertUrlTest,
-       ConvertToContentUrl_InvalidDownloads) {
+       ConvertToContentUrls_InvalidDownloads) {
   const base::FilePath downloads = GetDownloadsFolderForProfile(
       chromeos::ProfileHelper::Get()->GetProfileByUserIdHashForTest(
           "user2@gmail.com-hash"));
   base::RunLoop run_loop;
-  ConvertToContentUrl(CreateExternalURL(downloads.AppendASCII("a/b/c")),
+  ConvertToContentUrls(
+      std::vector<FileSystemURL>{
+          CreateExternalURL(downloads.AppendASCII("a/b/c"))},
       base::BindOnce(
-                          [](base::RunLoop* run_loop, const GURL& url) {
+          [](base::RunLoop* run_loop, const std::vector<GURL>& urls) {
             run_loop->Quit();
-                            EXPECT_EQ(GURL(), url);
+            ASSERT_EQ(1U, urls.size());
+            EXPECT_EQ(GURL(), urls[0]);
           },
           &run_loop));
   run_loop.Run();
 }
 
-TEST_F(FileManagerPathUtilConvertUrlTest, ConvertToContentUrl_Special) {
+TEST_F(FileManagerPathUtilConvertUrlTest, ConvertToContentUrls_Special) {
   base::RunLoop run_loop;
-  ConvertToContentUrl(
-      CreateExternalURL(drive_mount_point_.AppendASCII("a/b/c")),
+  ConvertToContentUrls(
+      std::vector<FileSystemURL>{
+          CreateExternalURL(drive_mount_point_.AppendASCII("a/b/c"))},
       base::BindOnce(
-          [](base::RunLoop* run_loop, const GURL& url) {
+          [](base::RunLoop* run_loop, const std::vector<GURL>& urls) {
             run_loop->Quit();
+            ASSERT_EQ(1U, urls.size());
             EXPECT_EQ(
                 GURL(
                     "content://org.chromium.arc.chromecontentprovider/"
                     "externalfile%3Adrive-user%2540gmail.com-hash%2Fa%2Fb%2Fc"),
-                url);
+                urls[0]);
           },
           &run_loop));
   run_loop.Run();
 }
 
 TEST_F(FileManagerPathUtilConvertUrlTest,
-       ConvertToContentUrl_ArcDocumentsProvider) {
+       ConvertToContentUrls_ArcDocumentsProvider) {
   // Add images_root/Download/photo.jpg to the fake file system.
   const char kAuthority[] = "com.android.providers.media.documents";
   fake_file_system_.AddDocument(arc::FakeFileSystemInstance::Document(
@@ -312,37 +325,67 @@ TEST_F(FileManagerPathUtilConvertUrlTest,
       kAuthority, "photo-id", "dir-id", "photo.jpg", "image/jpeg", 3, 33));
 
   base::RunLoop run_loop;
-  ConvertToContentUrl(
-      storage::FileSystemURL::CreateForTest(
+  ConvertToContentUrls(
+      std::vector<FileSystemURL>{FileSystemURL::CreateForTest(
           GURL(), storage::kFileSystemTypeArcDocumentsProvider,
           base::FilePath::FromUTF8Unsafe(
               "/special/arc-documents-provider/"
               "com.android.providers.media.documents/"
-              "images_root/Download/photo.jpg")),
+              "images_root/Download/photo.jpg"))},
       base::BindOnce(
-          [](base::RunLoop* run_loop, const GURL& url) {
+          [](base::RunLoop* run_loop, const std::vector<GURL>& urls) {
             run_loop->Quit();
+            ASSERT_EQ(1U, urls.size());
             EXPECT_EQ(GURL("content://com.android.providers.media.documents/"
                            "document/photo-id"),
-                      url);
+                      urls[0]);
           },
           &run_loop));
   run_loop.Run();
 }
 
 TEST_F(FileManagerPathUtilConvertUrlTest,
-       ConvertToContentUrl_ArcDocumentsProviderFileNotFound) {
+       ConvertToContentUrls_ArcDocumentsProviderFileNotFound) {
   base::RunLoop run_loop;
-  ConvertToContentUrl(storage::FileSystemURL::CreateForTest(
+  ConvertToContentUrls(
+      std::vector<FileSystemURL>{FileSystemURL::CreateForTest(
           GURL(), storage::kFileSystemTypeArcDocumentsProvider,
           base::FilePath::FromUTF8Unsafe(
               "/special/arc-documents-provider/"
               "com.android.providers.media.documents/"
-                              "images_root/Download/photo.jpg")),
+              "images_root/Download/photo.jpg"))},
       base::BindOnce(
-                          [](base::RunLoop* run_loop, const GURL& url) {
+          [](base::RunLoop* run_loop, const std::vector<GURL>& urls) {
             run_loop->Quit();
-                            EXPECT_EQ(GURL(""), url);
+            ASSERT_EQ(1U, urls.size());
+            EXPECT_EQ(GURL(""), urls[0]);
+          },
+          &run_loop));
+  run_loop.Run();
+}
+
+TEST_F(FileManagerPathUtilConvertUrlTest, ConvertToContentUrls_MultipeUrls) {
+  base::RunLoop run_loop;
+  ConvertToContentUrls(
+      std::vector<FileSystemURL>{
+          CreateExternalURL(base::FilePath::FromUTF8Unsafe("/invalid")),
+          CreateExternalURL(
+              base::FilePath::FromUTF8Unsafe("/media/removable/a/b/c")),
+          CreateExternalURL(drive_mount_point_.AppendASCII("a/b/c")),
+      },
+      base::BindOnce(
+          [](base::RunLoop* run_loop, const std::vector<GURL>& urls) {
+            run_loop->Quit();
+            ASSERT_EQ(3U, urls.size());
+            EXPECT_EQ(GURL(), urls[0]);  // Invalid URL.
+            EXPECT_EQ(
+                GURL("content://org.chromium.arc.removablemediaprovider/a/b/c"),
+                urls[1]);
+            EXPECT_EQ(
+                GURL(
+                    "content://org.chromium.arc.chromecontentprovider/"
+                    "externalfile%3Adrive-user%2540gmail.com-hash%2Fa%2Fb%2Fc"),
+                urls[2]);
           },
           &run_loop));
   run_loop.Run();

chrome/browser/exo_parts.cc

@@ -10,7 +10,6 @@
 #include <glib.h>
 #endif
 
-#include "base/barrier_closure.h"
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
@@ -73,20 +72,6 @@ void GetFileSystemUrlsFromPickle(
   }
 }
 
-// Helper function for |GetUrlsFromPickle|.
-void OnSingleUrlResolved(const base::RepeatingClosure& barrier_closure,
-                         std::vector<GURL>* out_urls,
-                         const GURL& url) {
-  out_urls->push_back(url);
-  barrier_closure.Run();
-}
-
-// Helper function for |GetUrlsFromPickle|.
-void OnAllUrlsResolved(exo::FileHelper::UrlsFromPickleCallback callback,
-                       std::unique_ptr<std::vector<GURL>> urls) {
-  std::move(callback).Run(*urls);
-}
-
 class ChromeFileHelper : public exo::FileHelper {
  public:
   ChromeFileHelper() = default;
@@ -118,19 +103,8 @@ class ChromeFileHelper : public exo::FileHelper {
       std::move(callback).Run(std::vector<GURL>());
       return;
     }
-
-    auto out_urls = std::make_unique<std::vector<GURL>>();
-    auto* out_urls_ptr = out_urls.get();
-    auto barrier = base::BarrierClosure(
-        file_system_urls.size(),
-        base::BindOnce(&OnAllUrlsResolved, std::move(callback),
-                       std::move(out_urls)));
-    auto convert_url_callback =
-        base::BindRepeating(&OnSingleUrlResolved, barrier, out_urls_ptr);
-    for (const auto& file_system_url : file_system_urls) {
-      file_manager::util::ConvertToContentUrl(file_system_url,
-                                              convert_url_callback);
-    }
+    file_manager::util::ConvertToContentUrls(file_system_urls,
+                                             std::move(callback));
   }
 };