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..e0386e7f4ba2e5a8fcfcd595c5ab03e31119b9a7 100644
--- a/chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc
+++ b/chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc
@@ -86,8 +86,14 @@ class ArcDownloadsWatcherService::DownloadsWatcher {
 
  private:
   // Called by base::FilePathWatcher to notify file changes.
+  // Wrapper around HandleFilePathChanged to pass the notification timestamp.
   void OnFilePathChanged(const base::FilePath& path, bool error);
 
+  // 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);
+
   // Scans files under |downloads_dir_| recursively and builds a map from file
   // paths (in Android filesystem) to last modified timestamps.
   TimestampMap BuildTimestampMap() const;
@@ -96,6 +102,7 @@ class ArcDownloadsWatcherService::DownloadsWatcher {
   base::FilePath downloads_dir_;
   std::unique_ptr<base::FilePathWatcher> watcher_;
   TimestampMap last_timestamp_map_;
+  base::Time last_update_time_;
 
   // 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 +113,9 @@ class ArcDownloadsWatcherService::DownloadsWatcher {
 
 ArcDownloadsWatcherService::DownloadsWatcher::DownloadsWatcher(
     const Callback& callback)
-    : callback_(callback), weak_ptr_factory_(this) {
+    : callback_(callback),
+      last_update_time_(base::Time()),
+      weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   downloads_dir_ = DownloadPrefs(ProfileManager::GetActiveUserProfile())
@@ -138,6 +147,20 @@ 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);
+  BrowserThread::PostTask(
+      FROM_HERE, base::Bind(&DownloadsWatcher::HandleFilePathChanged,
+                            weak_ptr_factory_.GetWeakPtr(), base::Time::Now()));
+}
+
+void ArcDownloadsWatcherService::DownloadsWatcher::HandleFilePathChanged(
+    base::Time notify_time) {
+  // Because this logic handles all updates and not just the update that
+  // triggered the callback we can return early if we have already processed
+  // updates to the directory after this callback was created.
+  if (notify_time < last_update_time_) {
+    return;
+  }
+  last_update_time_ = base::Time::Now();
 
   TimestampMap current_timestamp_map = BuildTimestampMap();

[hidehiko, 2016/09/07 01:44:32]

For the record.

As we chatted offline, because the heavy operation still blocks FILE thread, can
we move the heaviest part of this function into WorkerPool, with keeping
timestamp check in FILE thread? Something like...

OnFilePathChanged() {
  if (is_build_timestamp_map_running) {
    // Suspend the task.
    last_on_file_path_changed_time = Time::Now();
    return;
  }
  PostTaskAndReply(...,
                   Bind(HandleFilePathChanged, ...),
                   Bind(OnBuildTimestampMap, GetWeakPtr());
}

HandleFilePathChanged(...) {
  Time update_time = Time::Now();
  ... heavy operation ...
  return pair(update_time, timestamp_map);
}

OnBuildTimestampMap(update_time, timestamp_map) {
  last_timestamp_map_ = timestamp_map;
  if (last_on_file_path_changed_ > update_time) {
    // During the map building, another change had come. Re-run.
    PostTask(...);
  }
}