arc: Fix thread unsafe behavior of ArcDownloadsWatcherService

chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc

Index: chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc
diff --git a/chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc b/chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc
index cf63bf4dfe3933669f078652c92a5bcaa14a56a4..2c2235bf060195ea7981e5ac770d840d0f2dd26c 100644
--- a/chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc
+++ b/chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc
@@ -35,35 +35,35 @@ const base::FilePath::CharType kAndroidDownloadDir[] =
 
 // Compares two TimestampMaps and returns the list of file paths added/removed
 // or whose timestamp have changed.
-std::vector<base::FilePath> CollectChangedPaths(
+std::vector<std::string> CollectChangedPaths(

[hashimoto, 2016/09/01 07:34:20]

Semantically, base::FilePath suits better than std::string here, but
base::FilePath is not movable so it's bad for performance.
Which do you prefer?

[hidehiko, 2016/09/01 17:05:45]

Is it measurable? If so, it's good to note.
If not, I prefer cleaner semantic code, so base:FilePath, personally, but not
strong opinion in this case and either is fine.

Alternatively, you could add no-except-move-ctor for chrome-wide
vector<base::FilePath> better performance.

[hashimoto, 2016/09/02 03:56:00]

I just thought it might be nice to do as there is a performance issue in this
class, but I didn't measure the performance. 
If your preference is base::FilePath, let's stick to it as the performance
difference shouldn't be huge.

     const TimestampMap& timestamp_map_a,
     const TimestampMap& timestamp_map_b) {
-  std::vector<base::FilePath> changed_paths;
+  std::vector<std::string> changed_paths;
 
   TimestampMap::const_iterator iter_a = timestamp_map_a.begin();
   TimestampMap::const_iterator iter_b = timestamp_map_b.begin();
   while (iter_a != timestamp_map_a.end() && iter_b != timestamp_map_b.end()) {
     if (iter_a->first == iter_b->first) {
       if (iter_a->second != iter_b->second) {
-        changed_paths.emplace_back(iter_a->first);
+        changed_paths.emplace_back(iter_a->first.value());
       }
       ++iter_a;
       ++iter_b;
     } else if (iter_a->first < iter_b->first) {
-      changed_paths.emplace_back(iter_a->first);
+      changed_paths.emplace_back(iter_a->first.value());
       ++iter_a;
     } else {  // iter_a->first > iter_b->first
-      changed_paths.emplace_back(iter_b->first);
+      changed_paths.emplace_back(iter_b->first.value());
       ++iter_b;
     }
   }
 
   while (iter_a != timestamp_map_a.end()) {
-    changed_paths.emplace_back(iter_a->first);
+    changed_paths.emplace_back(iter_a->first.value());
     ++iter_a;
   }
   while (iter_b != timestamp_map_b.end()) {
-    changed_paths.emplace_back(iter_b->first);
+    changed_paths.emplace_back(iter_b->first.value());
     ++iter_b;
   }
 
@@ -77,7 +77,7 @@ std::vector<base::FilePath> CollectChangedPaths(
 class ArcDownloadsWatcherService::DownloadsWatcher {
  public:
   using Callback =
-      base::Callback<void(const std::vector<base::FilePath>& paths)>;
+      base::Callback<void(std::unique_ptr<mojo::Array<mojo::String>> paths)>;
 
   explicit DownloadsWatcher(const Callback& callback);
   ~DownloadsWatcher();
@@ -142,12 +142,19 @@ void ArcDownloadsWatcherService::DownloadsWatcher::OnFilePathChanged(
 
   TimestampMap current_timestamp_map = BuildTimestampMap();
 
-  std::vector<base::FilePath> changed_paths =
+  std::vector<std::string> changed_paths =
       CollectChangedPaths(last_timestamp_map_, current_timestamp_map);
 
   last_timestamp_map_ = std::move(current_timestamp_map);
 
-  callback_.Run(changed_paths);
+  auto mojo_paths =
+      base::MakeUnique<mojo::Array<mojo::String>>(changed_paths.size());

[hidehiko, 2016/09/01 17:05:45]

Is it necessary to be unique_ptr? I thought mojo::Array supports std::move so it
should work with base::Passed()?

[hashimoto, 2016/09/02 03:56:00]

Thank you for pointing this out.
I wasn't aware of the fact that base::Passed can work with classes other than
scoped_ptr (which no longer exists).

+  for (size_t i = 0; i < changed_paths.size(); ++i) {
+    (*mojo_paths)[i] = std::move(changed_paths[i]);
+  }
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(callback_, base::Passed(std::move(mojo_paths))));
 }
 
 TimestampMap ArcDownloadsWatcherService::DownloadsWatcher::BuildTimestampMap()
@@ -217,18 +224,13 @@ void ArcDownloadsWatcherService::StopWatchingDownloads() {
 }
 
 void ArcDownloadsWatcherService::OnDownloadsChanged(
-    const std::vector<base::FilePath>& paths) {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+    std::unique_ptr<mojo::Array<mojo::String>> paths) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   auto* instance = arc_bridge_service()->file_system()->instance();
   if (!instance)
     return;
-
-  mojo::Array<mojo::String> mojo_paths(paths.size());
-  for (size_t i = 0; i < paths.size(); ++i) {
-    mojo_paths[i] = paths[i].value();
-  }
-  instance->RequestMediaScan(std::move(mojo_paths));
+  instance->RequestMediaScan(std::move(*paths));
 }
 
 }  // namespace arc