Don't block FILE thread for media scanning

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 cb43c4c445b344f5b02f3a5048cf2c070f51d20c..06f715d5c36a5086b6618189a976c5b1d6718f45 100644
--- a/chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc
+++ b/chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc
@@ -70,6 +70,37 @@ std::vector<base::FilePath> CollectChangedPaths(
   return changed_paths;
 }
 
+// Scans files under |downloads_dir| recursively and builds a map from file
+// paths (in Android filesystem) to last modified timestamps.
+TimestampMap BuildTimestampMap(base::FilePath downloads_dir) {
+  DCHECK(!downloads_dir.EndsWithSeparator());
+  TimestampMap timestamp_map;
+
+  // Enumerate normal files only; directories and symlinks are skipped.
+  base::FileEnumerator enumerator(downloads_dir, true,
+                                  base::FileEnumerator::FILES);
+  for (base::FilePath cros_path = enumerator.Next(); !cros_path.empty();
+       cros_path = enumerator.Next()) {
+    // Android file path can be obtained by replacing |downloads_dir| prefix
+    // with |kAndroidDownloadDir|.
+    const base::FilePath& android_path =
+        base::FilePath(kAndroidDownloadDir)
+            .Append(
+                cros_path.value().substr(downloads_dir.value().length() + 1));
+    const base::FileEnumerator::FileInfo& info = enumerator.GetInfo();
+    timestamp_map[android_path] = info.GetLastModifiedTime();
+  }
+  return timestamp_map;
+}
+
+std::pair<base::Time, TimestampMap> BuildTimestampMapCallback(
+    base::FilePath downloads_dir) {
+  base::Time update_time = base::Time::Now();
+  TimestampMap current_timestamp_map = BuildTimestampMap(downloads_dir);
+  return std::pair<base::Time, TimestampMap>(update_time,

[Shuhei Takahashi, 2016/09/08 03:43:48]

FYI, std::make_pair is useful to avoid explicit template parameters:
http://www.cplusplus.com/reference/utility/make_pair/

[dspaid, 2016/09/08 03:59:56]

Done.

+                                             current_timestamp_map);

[Shuhei Takahashi, 2016/09/08 03:43:48]

IIUC you need std::move() here to avoid copy.

[dspaid, 2016/09/08 03:59:56]

Done.

+}
+
 }  // namespace
 
 // The core part of ArcDownloadsWatcherService to watch for file changes in
@@ -86,16 +117,25 @@ class ArcDownloadsWatcherService::DownloadsWatcher {
 
  private:
   // Called by base::FilePathWatcher to notify file changes.
+  // Wrapper around HandleFilePathChanged to pass the notification timestamp.

[Shuhei Takahashi, 2016/09/08 03:43:48]

This line is outdated.

[dspaid, 2016/09/08 03:59:56]

Done.

   void OnFilePathChanged(const base::FilePath& path, bool error);
 
-  // Scans files under |downloads_dir_| recursively and builds a map from file
-  // paths (in Android filesystem) to last modified timestamps.
-  TimestampMap BuildTimestampMap() const;
+  // If there has not been a full scan of updated files since notify_time,
+  // update the last_timestamp_map_ and send any new/changed files to the
+  // android media scanner.
+  void HandleFilePathChanged(base::Time notify_time);

[Shuhei Takahashi, 2016/09/08 03:43:48]

This function is no longer defined.

[dspaid, 2016/09/08 03:59:56]

Done.

+
+  void OnBuildTimestampMap(
+      std::pair<base::Time, TimestampMap> timestamp_and_map);
 
   Callback callback_;
   base::FilePath downloads_dir_;
   std::unique_ptr<base::FilePathWatcher> watcher_;
   TimestampMap last_timestamp_map_;
+  // The timestamp of the last OnFilePathChanged callback received.
+  base::Time last_update_time_;

[Shuhei Takahashi, 2016/09/08 03:43:48]

How about using "bool pending_task_" instead? Though the logic will change
slightly, efficiency is almost same, and the code will get easier to read.

[dspaid, 2016/09/08 03:59:56]

The small timing difference actually has a reasonable performance gain since we
receive up to 3 events per action almost simultaneously.  With the timestamp
implementation these should be batched up into a single call in most cases,
whereas the boolean implementation would guarantee two full scans per action.

[Shuhei Takahashi, 2016/09/08 04:25:48]

I see. But then I think it's a bit optimistic to assume such three events occur
before the callback is invoked. How about inserting some delay (1000ms for
example) before the callback?

[dspaid, 2016/09/09 02:10:45]

Experimentally I'm seeing a reasonable success rate (~68% of operations with 3
events are being deduped into a single update).  I was looking for something
like PostDelayedTask to introduce delay, but there doesn't seem to be anything
to use with PostTaskAndReplyWithResult semantics.  I'm generally skeptical of
solutions that involve directly calling sleep, but am not sure what the common
practice in chromium is.

[Shuhei Takahashi, 2016/09/09 03:18:51]

It is best if we have PostDelayedTaskAndReplyWithResult, but it apparently does
not exist. Maybe you can define an intermediate callback function and calling
PostDelayedTask and PostTaskAndReplyWithResult in order?

+  // Whether or not there is an outstanding task to update last_timestamp_map_.
+  bool outstanding_task_;
 
   // Note: This should remain the last member so it'll be destroyed and
   // invalidate the weak pointers before any other members are destroyed.
@@ -106,7 +146,10 @@ class ArcDownloadsWatcherService::DownloadsWatcher {
 
 ArcDownloadsWatcherService::DownloadsWatcher::DownloadsWatcher(
     const Callback& callback)
-    : callback_(callback), weak_ptr_factory_(this) {
+    : callback_(callback),
+      last_update_time_(base::Time()),
+      outstanding_task_(false),
+      weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   downloads_dir_ = DownloadPrefs(ProfileManager::GetActiveUserProfile())
@@ -123,7 +166,10 @@ void ArcDownloadsWatcherService::DownloadsWatcher::Start() {
 
   // Initialize with the current timestamp map and avoid initial notification.
   // It is not needed since MediaProvider scans whole storage area on boot.
-  last_timestamp_map_ = BuildTimestampMap();
+  last_update_time_ = base::Time::Now();
+  last_timestamp_map_ = BuildTimestampMap(downloads_dir_);
+
+  outstanding_task_ = false;
 
   watcher_ = base::MakeUnique<base::FilePathWatcher>();
   // On Linux, base::FilePathWatcher::Watch() always returns true.
@@ -138,9 +184,21 @@ void ArcDownloadsWatcherService::DownloadsWatcher::OnFilePathChanged(
   // On Linux, |error| is always false. Also, |path| is always the same path
   // as one given to FilePathWatcher::Watch().
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  if (!outstanding_task_) {
+    base::PostTaskAndReplyWithResult(
+        BrowserThread::GetBlockingPool(), FROM_HERE,
+        base::Bind(&BuildTimestampMapCallback, downloads_dir_),
+        base::Bind(&DownloadsWatcher::OnBuildTimestampMap,
+                   weak_ptr_factory_.GetWeakPtr()));
+    outstanding_task_ = true;
+  } else {
+    last_update_time_ = base::Time::Now();
+  }
+}
 
-  TimestampMap current_timestamp_map = BuildTimestampMap();
-
+void ArcDownloadsWatcherService::DownloadsWatcher::OnBuildTimestampMap(
+    std::pair<base::Time, TimestampMap> timestamp_and_map) {
+  TimestampMap current_timestamp_map = timestamp_and_map.second;

[Shuhei Takahashi, 2016/09/08 03:43:48]

Could you insert DCHECK_CURRENTLY_ON?

[Shuhei Takahashi, 2016/09/08 03:43:48]

Also, could you insert DCHECK(outstanding_task_)?

[dspaid, 2016/09/08 03:59:56]

Done.

[dspaid, 2016/09/08 03:59:57]

Done.

   std::vector<base::FilePath> changed_paths =
       CollectChangedPaths(last_timestamp_map_, current_timestamp_map);
 
@@ -153,28 +211,14 @@ void ArcDownloadsWatcherService::DownloadsWatcher::OnFilePathChanged(
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(callback_, base::Passed(std::move(mojo_paths))));
-}
-
-TimestampMap ArcDownloadsWatcherService::DownloadsWatcher::BuildTimestampMap()
-    const {
-  DCHECK(!downloads_dir_.EndsWithSeparator());
-  TimestampMap timestamp_map;
-
-  // Enumerate normal files only; directories and symlinks are skipped.
-  base::FileEnumerator enumerator(downloads_dir_, true,
-                                  base::FileEnumerator::FILES);
-  for (base::FilePath cros_path = enumerator.Next(); !cros_path.empty();
-       cros_path = enumerator.Next()) {
-    // Android file path can be obtained by replacing |downloads_dir_| prefix
-    // with |kAndroidDownloadDir|.
-    const base::FilePath& android_path =
-        base::FilePath(kAndroidDownloadDir)
-            .Append(
-                cros_path.value().substr(downloads_dir_.value().length() + 1));
-    const base::FileEnumerator::FileInfo& info = enumerator.GetInfo();
-    timestamp_map[android_path] = info.GetLastModifiedTime();
+  if (last_update_time_ > timestamp_and_map.first) {
+    base::PostTaskAndReplyWithResult(
+        BrowserThread::GetBlockingPool(), FROM_HERE,
+        base::Bind(&BuildTimestampMapCallback, downloads_dir_),
+        base::Bind(&DownloadsWatcher::OnBuildTimestampMap,
+                   weak_ptr_factory_.GetWeakPtr()));
   }
-  return timestamp_map;
+  outstanding_task_ = false;

[Shuhei Takahashi, 2016/09/08 03:43:48]

We must keep |outstanding_task_| true when there was a pending task and
PostTaskAndReplyWithResult() was called.

[dspaid, 2016/09/08 03:59:56]

Done.

 }
 
 ArcDownloadsWatcherService::ArcDownloadsWatcherService(