DevTools: improve DevTools file watcher to send added and removed paths

chrome/browser/devtools/devtools_file_watcher.cc

Index: chrome/browser/devtools/devtools_file_watcher.cc
diff --git a/chrome/browser/devtools/devtools_file_watcher.cc b/chrome/browser/devtools/devtools_file_watcher.cc
index e5af5a5f198df8f249f0c90fead55bb3a152d088..4652b7cf154c47d973cbec4a45454e06387534b3 100644
--- a/chrome/browser/devtools/devtools_file_watcher.cc
+++ b/chrome/browser/devtools/devtools_file_watcher.cc
@@ -13,6 +13,7 @@
 #include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/files/file_path_watcher.h"
+#include "base/files/file_util.h"
 #include "base/memory/ref_counted.h"
 #include "content/public/browser/browser_thread.h"
 
@@ -38,13 +39,15 @@ class DevToolsFileWatcher::SharedFileWatcher :
       DevToolsFileWatcher::SharedFileWatcher>;
   ~SharedFileWatcher();
 
+  using FilePathTimesMap = std::map<base::FilePath, base::Time>;
+  void GetModificationTimes(const base::FilePath& path,
+                            FilePathTimesMap* file_path_times);
   void DirectoryChanged(const base::FilePath& path, bool error);
   void DispatchNotifications();
 
   std::vector<DevToolsFileWatcher*> listeners_;
   std::map<base::FilePath, std::unique_ptr<base::FilePathWatcher>> watchers_;
-  using FilePathTimesMap = std::map<base::FilePath, base::Time>;
-  FilePathTimesMap file_path_times_;
+  std::map<base::FilePath, FilePathTimesMap> file_path_times_;
   std::set<base::FilePath> pending_paths_;
   base::Time last_event_time_;
   base::TimeDelta last_dispatch_cost_;
@@ -84,11 +87,17 @@ void DevToolsFileWatcher::SharedFileWatcher::AddWatch(
   if (!success)
     return;
 
+  GetModificationTimes(path, &file_path_times_[path]);
+}
+
+void DevToolsFileWatcher::SharedFileWatcher::GetModificationTimes(
+    const base::FilePath& path,
+    FilePathTimesMap* times_map) {
   base::FileEnumerator enumerator(path, true, base::FileEnumerator::FILES);
   base::FilePath file_path = enumerator.Next();
   while (!file_path.empty()) {
     base::FileEnumerator::FileInfo file_info = enumerator.GetInfo();
-    file_path_times_[file_path] = file_info.GetLastModifiedTime();
+    (*times_map)[file_path] = file_info.GetLastModifiedTime();
     file_path = enumerator.Next();
   }
 }
@@ -121,26 +130,41 @@ void DevToolsFileWatcher::SharedFileWatcher::DirectoryChanged(
 }
 
 void DevToolsFileWatcher::SharedFileWatcher::DispatchNotifications() {
+  if (!pending_paths_.size())
+    return;
   base::Time start = base::Time::Now();
+  std::vector<std::string> added_paths;
+  std::vector<std::string> removed_paths;
   std::vector<std::string> changed_paths;
-  for (auto path : pending_paths_) {
-    base::FileEnumerator enumerator(path, true, base::FileEnumerator::FILES);
-    base::FilePath file_path = enumerator.Next();
-    while (!file_path.empty()) {
-      base::FileEnumerator::FileInfo file_info = enumerator.GetInfo();
-      base::Time new_time = file_info.GetLastModifiedTime();
-      if (file_path_times_[file_path] != new_time) {
-        file_path_times_[file_path] = new_time;
-        changed_paths.push_back(file_path.AsUTF8Unsafe());
+
+  for (const auto& path : pending_paths_) {
+    FilePathTimesMap& old_times = file_path_times_[path];
+    FilePathTimesMap current_times;
+    GetModificationTimes(path, &current_times);
+    for (const auto& it : current_times) {

[dgozman, 2016/10/04 04:41:01]

I usually call it |pair| or |path_time|.

[lushnikov, 2016/10/04 15:56:35]

|path_time| is really nice, thanks

+      auto old_timestamp = old_times.find(it.first);
+      if (old_timestamp == old_times.end()) {
+        old_times[it.first] = it.second;

[dgozman, 2016/10/04 04:41:01]

Don't modify, you do swap below.

[lushnikov, 2016/10/04 15:56:35]

My bad. Done.

+        added_paths.push_back(it.first.AsUTF8Unsafe());
+      } else if (old_timestamp->second != it.second) {
+        old_timestamp->second = it.second;

[dgozman, 2016/10/04 04:41:01]

ditto

[lushnikov, 2016/10/04 15:56:35]

Done.

+        changed_paths.push_back(it.first.AsUTF8Unsafe());
+      }
+    }
+    for (const auto& it : old_times) {

[dgozman, 2016/10/04 04:41:01]

I'd rather do:

for (const auto& path_time : old_times) {
  base::FilePath& path = path_time.first;
  if (current_times.find(path) == current_times.end())
    removed_paths.push_back(path.AsUTF8Unsafe());
}

[lushnikov, 2016/10/04 15:56:35]

This is much more clear, thanks

+      auto new_timestamp = current_times.find(it.first);
+      if (new_timestamp == current_times.end()) {
+        removed_paths.push_back(it.first.AsUTF8Unsafe());
       }
-      file_path = enumerator.Next();
     }
+    old_times.swap(current_times);
   }
   pending_paths_.clear();
 
   for (auto* watcher : listeners_) {
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                            base::Bind(watcher->callback_, changed_paths));
+                            base::Bind(watcher->callback_, changed_paths,
+                                       added_paths, removed_paths));
   }
   last_dispatch_cost_ = base::Time::Now() - start;
 }