Changed api to notify when watched directory is deleted.

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

 // Copyright 2014 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/extensions/file_manager/event_router.h"
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/prefs/pref_change_registrar.h"
@@ skipping 368 matching lines @@
 
 EventRouter::DriveJobInfoWithStatus::DriveJobInfoWithStatus(
     const drive::JobInfo& info, const std::string& status)
     : job_info(info), status(status) {
 }
 
 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();
 }
 
 EventRouter::~EventRouter() {
 }
 
 void EventRouter::Shutdown() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ skipping 305 matching lines @@
   BroadcastEvent(profile_,
                  file_manager_private::OnFileTransfersUpdated::kEventName,
                  file_manager_private::OnFileTransfersUpdated::Create(status));
 }
 
 void EventRouter::OnDirectoryChanged(const base::FilePath& drive_path) {
   HandleFileWatchNotification(NULL, drive_path, false);
 }
 
 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++) {
     HandleFileWatchNotification(&(it->second), it->first, false);
   }
 }
 
 void EventRouter::OnDriveSyncError(drive::file_system::DriveSyncErrorType type,
                                    const base::FilePath& drive_path) {
   file_manager_private::DriveSyncErrorEvent event;
@@ skipping 51 matching lines @@
                                list,
                                got_error,
                                iter->second->GetExtensionIds());
 }
 
 void EventRouter::DispatchDirectoryChangeEvent(
     const base::FilePath& virtual_path,
     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;
   }
   linked_ptr<drive::FileChange> changes;
   if (list)
     changes.reset(new drive::FileChange(*list));  // Copy
 
   for (size_t i = 0; i < extension_ids.size(); ++i) {
     std::string* extension_id = new std::string(extension_ids[i]);
@@ skipping 156 matching lines @@
 void EventRouter::Observe(int type,
                           const content::NotificationSource& source,
                           const content::NotificationDetails& details) {
   if (type == chrome::NOTIFICATION_PROFILE_ADDED) {
     Profile* const added_profile = content::Source<Profile>(source).ptr();
     if (!added_profile->IsOffTheRecord())
       GrantAccessForAddedProfileToRunningInstance(added_profile, profile_);
   }
 }
 
+void EventRouter::SetDispatchDirectoryChangeEventImplForTesting(
+    const DispatchDirectoryChangeEventImplCallback& callback) {
+  dispatch_directory_change_event_impl_ = callback;
+}
+
 }  // namespace file_manager