Don't block FILE thread for media scanning

chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/arc/arc_downloads_watcher_service.h"
 
 #include <map>
 #include <memory>
 #include <utility>
 
@@ skipping 15 matching lines @@
 // Mapping from Android file paths to last modified timestamps.
 using TimestampMap = std::map<base::FilePath, base::Time>;
 
 namespace arc {
 
 namespace {
 
 const base::FilePath::CharType kAndroidDownloadDir[] =
     FILE_PATH_LITERAL("/storage/emulated/0/Download");
 
+// How long to wait for new inotify events before building the updated timestamp
+// map.
+const base::TimeDelta kBuildTimestampMapDelay =
+    base::TimeDelta::FromMilliseconds(1000);

[hidehiko, 2016/09/12 05:44:10]

Optional: 1s sounds a bit longer delay...?
Anyway, could you comment the heuristics how to decide 1s?

[dspaid, 2016/09/12 05:56:51]

Completely arbitrary.  Testing shows that we can actually get away with
something as short as 5ms for just deduping individual file events, but nya@
suggested a longer delay since scanning media files is a very low-priority task
(and 1s would let us dedupe multi-file events like unzipping/copying).

+
 // Compares two TimestampMaps and returns the list of file paths added/removed
 // or whose timestamp have changed.
 std::vector<base::FilePath> CollectChangedPaths(
     const TimestampMap& timestamp_map_a,
     const TimestampMap& timestamp_map_b) {
   std::vector<base::FilePath> 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()) {
@@ skipping 17 matching lines @@
     ++iter_a;
   }
   while (iter_b != timestamp_map_b.end()) {
     changed_paths.emplace_back(iter_b->first);
     ++iter_b;
   }
 
   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(

[hidehiko, 2016/09/12 05:44:10]

Maybe, FilePath::AppendRelativePath() is what you want?

[dspaid, 2016/09/12 05:56:51]

Done.

+                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::TimeTicks, TimestampMap> BuildTimestampMapCallback(
+    base::FilePath downloads_dir) {
+  base::TimeTicks snapshot_time = base::TimeTicks::Now();

[hidehiko, 2016/09/12 05:44:10]

Could you comment that,
- BuildTimestampMap take a bit longer time.
- snapshot_time must be taken *before* building map.

[dspaid, 2016/09/12 05:56:52]

Done.

+  TimestampMap current_timestamp_map = BuildTimestampMap(downloads_dir);
+  return std::make_pair(snapshot_time, std::move(current_timestamp_map));
+}
+
 }  // namespace
 
 // The core part of ArcDownloadsWatcherService to watch for file changes in
 // Downloads directory.
 class ArcDownloadsWatcherService::DownloadsWatcher {
  public:
   using Callback = base::Callback<void(mojo::Array<mojo::String> paths)>;
 
   explicit DownloadsWatcher(const Callback& callback);
   ~DownloadsWatcher();
 
   // Starts watching Downloads directory.
   void Start();
 
  private:
   // Called by base::FilePathWatcher to notify file changes.
+  // Kicks off the update of last_timestamp_map_ if one is not already in
+  // progress.
   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;
+  // Called with a delay to allow additional inotify events for the same user
+  // action to queue up so that they can be dealt with in batch.
+  void DelayBuildTimestampMap();
+
+  // Called after a new timestamp map has been created and causes any recently
+  // modified files to be sent to the media scanner.
+  void OnBuildTimestampMap(
+      std::pair<base::TimeTicks, 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::TimeTicks last_notify_time_;
+  // 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.
   base::WeakPtrFactory<DownloadsWatcher> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadsWatcher);
 };
 
 ArcDownloadsWatcherService::DownloadsWatcher::DownloadsWatcher(
     const Callback& callback)
-    : callback_(callback), weak_ptr_factory_(this) {
+    : callback_(callback),
+      last_notify_time_(base::TimeTicks()),
+      outstanding_task_(false),
+      weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   downloads_dir_ = DownloadPrefs(ProfileManager::GetActiveUserProfile())
                        .GetDefaultDownloadDirectoryForProfile()
                        .StripTrailingSeparators();
 }
 
 ArcDownloadsWatcherService::DownloadsWatcher::~DownloadsWatcher() {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 }
 
 void ArcDownloadsWatcherService::DownloadsWatcher::Start() {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
   // 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_notify_time_ = base::TimeTicks::Now();
+  last_timestamp_map_ = BuildTimestampMap(downloads_dir_);
+
+  outstanding_task_ = false;

[hidehiko, 2016/09/12 05:44:10]

Clarification: what is this for? Here, outstandig_task_ should be false always?
If this is for just-in-case purpose, DCHECK sounds better I think?

[dspaid, 2016/09/12 05:56:51]

Done.

 
   watcher_ = base::MakeUnique<base::FilePathWatcher>();
   // On Linux, base::FilePathWatcher::Watch() always returns true.
   watcher_->Watch(downloads_dir_, true,
                   base::Bind(&DownloadsWatcher::OnFilePathChanged,
                              weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ArcDownloadsWatcherService::DownloadsWatcher::OnFilePathChanged(
     const base::FilePath& path,
     bool error) {
   // 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_) {
+    outstanding_task_ = true;
+    BrowserThread::PostDelayedTask(
+        BrowserThread::FILE, FROM_HERE,
+        base::Bind(&DownloadsWatcher::DelayBuildTimestampMap,
+                   weak_ptr_factory_.GetWeakPtr()),
+        kBuildTimestampMapDelay);
+  } else {
+    last_notify_time_ = base::TimeTicks::Now();
+  }
+}
 
-  TimestampMap current_timestamp_map = BuildTimestampMap();
+void ArcDownloadsWatcherService::DownloadsWatcher::DelayBuildTimestampMap() {
+  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  DCHECK(outstanding_task_);
+  base::PostTaskAndReplyWithResult(
+      BrowserThread::GetBlockingPool(), FROM_HERE,
+      base::Bind(&BuildTimestampMapCallback, downloads_dir_),
+      base::Bind(&DownloadsWatcher::OnBuildTimestampMap,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
 
+void ArcDownloadsWatcherService::DownloadsWatcher::OnBuildTimestampMap(
+    std::pair<base::TimeTicks, TimestampMap> timestamp_and_map) {
+  DCHECK_CURRENTLY_ON(BrowserThread::FILE);

[Shuhei Takahashi, 2016/09/12 04:29:13]

This should be not true, it's on blocking pool.

[hidehiko, 2016/09/12 05:44:10]

This should be on FILE thread, IIUC.

[Shuhei Takahashi, 2016/09/12 05:46:00]

Oops, I misread the code. Please don't mind.

+  DCHECK(outstanding_task_);
+  base::TimeTicks snapshot_time = timestamp_and_map.first;
+  TimestampMap current_timestamp_map = timestamp_and_map.second;

[Shuhei Takahashi, 2016/09/12 04:29:13]

Could you use std::move() here?

[dspaid, 2016/09/12 05:56:52]

Done.

   std::vector<base::FilePath> changed_paths =
       CollectChangedPaths(last_timestamp_map_, current_timestamp_map);
 
   last_timestamp_map_ = std::move(current_timestamp_map);
 
   mojo::Array<mojo::String> mojo_paths(changed_paths.size());
   for (size_t i = 0; i < changed_paths.size(); ++i) {
     mojo_paths[i] = changed_paths[i].value();
   }
   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_notify_time_ > snapshot_time) {
+    base::PostTaskAndReplyWithResult(
+        BrowserThread::GetBlockingPool(), FROM_HERE,
+        base::Bind(&BuildTimestampMapCallback, downloads_dir_),
+        base::Bind(&DownloadsWatcher::OnBuildTimestampMap,
+                   weak_ptr_factory_.GetWeakPtr()));
+  } else {
+    outstanding_task_ = false;
   }
-  return timestamp_map;
 }
 
 ArcDownloadsWatcherService::ArcDownloadsWatcherService(
     ArcBridgeService* bridge_service)
     : ArcService(bridge_service), weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   arc_bridge_service()->file_system()->AddObserver(this);
 }
 
 ArcDownloadsWatcherService::~ArcDownloadsWatcherService() {
@@ skipping 37 matching lines @@
     mojo::Array<mojo::String> paths) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   auto* instance = arc_bridge_service()->file_system()->instance();
   if (!instance)
     return;
   instance->RequestMediaScan(std::move(paths));
 }
 
 }  // namespace arc