Changed api to notify when watched directory is deleted.

chrome/browser/chromeos/extensions/file_manager/event_router.cc

Index: chrome/browser/chromeos/extensions/file_manager/event_router.cc
diff --git a/chrome/browser/chromeos/extensions/file_manager/event_router.cc b/chrome/browser/chromeos/extensions/file_manager/event_router.cc
index 9e0604cc8e26b0848f575559e66294274d4872c0..04b68c1191d9316c348a99d8e93793fb31d791fe 100644
--- a/chrome/browser/chromeos/extensions/file_manager/event_router.cc
+++ b/chrome/browser/chromeos/extensions/file_manager/event_router.cc
@@ -386,6 +386,9 @@ EventRouter::EventRouter(Profile* profile)
     : pref_change_registrar_(new PrefChangeRegistrar),
       profile_(profile),
       device_event_router_(new DeviceEventRouterImpl(profile)),
+      dispatch_directory_change_event_impl_(
+          base::Bind(&EventRouter::DispatchDirectoryChangeEventImpl,
+                     base::Unretained(this))),
       weak_factory_(this) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   ObserveEvents();
@@ -711,15 +714,52 @@ void EventRouter::OnDirectoryChanged(const base::FilePath& drive_path) {
 }
 
 void EventRouter::OnFileChanged(const drive::FileChange& changed_files) {
+  // For deletion of a directory, onFileChanged gets different changed_files.
+  // We solve the difference in this method.
+  //
+  // /a/b is watched, and /a is deleted from Drive (e.g. from Web).
+  // 1. /a/b DELETE:DIRECTORY
+  // 2. /a DELETE:DIRECTORY
+  //
+  // /a/b is watched, and /a is deleted from Files.app.
+  // 1. /a DELETE:DIRECTORY
   typedef std::map<base::FilePath, drive::FileChange> FileChangeMap;
+  typedef drive::FileChange::ChangeList::List FileChangeList;
 
   FileChangeMap map;
   const drive::FileChange::Map& changed_file_map = changed_files.map();
-  for (drive::FileChange::Map::const_iterator it = changed_file_map.begin();
-       it != changed_file_map.end();
-       it++) {
-    const base::FilePath& path = it->first;
-    map[path.DirName()].Update(path, it->second);
+  for (drive::FileChange::Map::const_iterator file_it =

[mtomasz, 2014/10/21 02:13:52]

nit: I was just told that we can use some cool C++11 features, as auto and
in-range for loops. How about using it here for better readability?

http://chromium-cpp.appspot.com/

[yawano, 2014/10/21 05:48:48]

Done.

+           changed_file_map.begin();
+       file_it != changed_file_map.end(); file_it++) {
+    bool contains_directory_deletion = false;
+    const FileChangeList list = file_it->second.list();
+    for (FileChangeList::const_iterator change_it = list.begin();
+         change_it != list.end(); ++change_it) {
+      if (change_it->IsDirectory() && change_it->IsDelete()) {
+        contains_directory_deletion = true;
+        break;
+      }
+    }
+
+    const base::FilePath& path = file_it->first;
+    map[path.DirName()].Update(path, file_it->second);

[mtomasz, 2014/10/21 02:13:52]

I don't really understand this logic, including the original code. In the for
loop we add new entries to the map. So, if we have: /c DELETE:DIRECTORY, then we
will add / DELETE:DIRECTORY. Is it right? That seems wrong, since / is not
deleted. Also, we add elements to the map without resetting the iterator, so we
may or may not iterate them (depending if they are added before or after the
current iterator).

I may be wrong here. Could you double check if the code logic is correct since
you work on it?

This code seems to be introduced by this patch:
https://codereview.chromium.org/343073003.

[yawano, 2014/10/21 05:48:48]

No, file change itself is not changed. HandleFileWatchNotification requres the
directory which file change had happend in order to matching the watched
directories.

For example, if /c {DELETE:DIRECTORY} comes we create the following "map".

map[/] = /c {DELETE:DIRECTORY}

and it is passed as "a change had happend in / with /c {DELETE:DIRECTORY}".

In this cl we extend this to create the following map.

map[/] = /c {DELETE:DIRECTORY}
map[/c] = /c {DELETE:DIRECTORY}

The second question about adding elements to the map is not the problem here,
since the iterator(file_it) is not a iterator of the "map". changed_file_map and
"map" are different map.

+
+    // If the change is a deletion of a directory, it may have deleted a
+    // watched directory.
+    // e.g. When /a is deleted, it means /a/b is also deleted.
+    if (contains_directory_deletion) {
+      // Expand the deleted directory path with watched paths.
+      for (WatcherMap::const_iterator file_watchers_it =
+               file_watchers_.lower_bound(path);
+           file_watchers_it != file_watchers_.end() &&
+               file_watchers_it->first.value().find(path.value()) == 0;
+           ++file_watchers_it) {
+        // Set an empty change list.
+        drive::FileChange::ChangeList change_list;
+        map[file_watchers_it->first].Update(file_watchers_it->first,
+                                            change_list);
+      }
+    }
   }
 
   for (FileChangeMap::const_iterator it = map.begin(); it != map.end(); it++) {
@@ -791,6 +831,13 @@ void EventRouter::DispatchDirectoryChangeEvent(
     const drive::FileChange* list,
     bool got_error,
     const std::vector<std::string>& extension_ids) {
+  dispatch_directory_change_event_impl_.Run(virtual_path, list, got_error,
+                                            extension_ids);
+}
+
+void EventRouter::DispatchDirectoryChangeEventImpl(
+    const base::FilePath& virtual_path, const drive::FileChange* list,
+    bool got_error, const std::vector<std::string>& extension_ids) {
   if (!profile_) {
     NOTREACHED();
     return;
@@ -967,4 +1014,9 @@ void EventRouter::Observe(int type,
   }
 }
 
+void EventRouter::SetDispatchDirectoryChangeEventImplForTesting(
+    const DispatchDirectoryChangeEventImplCallback& callback) {
+  dispatch_directory_change_event_impl_ = callback;
+}
+
 }  // namespace file_manager