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 cf63bf4dfe3933669f078652c92a5bcaa14a56a4..343dd5aba7c1c4f78d11884b6df1a057608d7e9a 100644
--- a/chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc
+++ b/chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc
@@ -30,6 +30,10 @@ namespace arc {
 
 namespace {
 
+// ID used to ensure sequential execution of HandleFilePathChanged.
+// This is sequential to prevent two instances from updating last_update_time_.
+const char kSequenceId[] = "arc_downloads_watcher";
+
 const base::FilePath::CharType kAndroidDownloadDir[] =
     FILE_PATH_LITERAL("/storage/emulated/0/Download");
 
@@ -87,8 +91,15 @@ class ArcDownloadsWatcherService::DownloadsWatcher {
 
  private:
   // Called by base::FilePathWatcher to notify file changes.
+  // Wrapper around HandleFilePathChanged to pass the notification timestamp
+  // and move the work off the FILE thread.
   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;
@@ -97,6 +108,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.
@@ -107,7 +119,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())
@@ -139,6 +153,18 @@ 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::PostBlockingPoolSequencedTask(
+      kSequenceId, FROM_HERE,
+      base::Bind(&DownloadsWatcher::HandleFilePathChanged,
+                 weak_ptr_factory_.GetWeakPtr(), base::Time::Now()));

[Shuhei Takahashi, 2016/09/05 09:28:23]

Weak pointers can be dereferenced only on FILE thread, but this callback would
cause it to be dereferenced on the blocking pool task runner. I guess DCHECK
will fail.

See: https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?l=49

[dspaid, 2016/09/07 01:16:56]

Thanks for the reminder.  I couldn't really find a thread safe way to get this
working so I've moved it back to the FILE thread.  However the rest of the
optimizations should still keep it from getting too bad.

+}
+
+void ArcDownloadsWatcherService::DownloadsWatcher::HandleFilePathChanged(
+    base::Time notify_time) {
+  if (notify_time < last_update_time_) {

[Shuhei Takahashi, 2016/09/05 09:28:23]

Please add a comment about the purpose of this check and how it works, this is a
bit tricky :)

[dspaid, 2016/09/07 01:16:56]

Done.

+    return;
+  }
+  last_update_time_ = base::Time::Now();
 
   TimestampMap current_timestamp_map = BuildTimestampMap();