Delete the installation folder after deleting its contents.

chrome/installer/setup/uninstall.cc

Index: chrome/installer/setup/uninstall.cc
diff --git a/chrome/installer/setup/uninstall.cc b/chrome/installer/setup/uninstall.cc
index f4c4d70a253314a8fbfd68c38f7f9964a60ae4f5..206ea493eb25e426229e726238cc0eb183816830 100644
--- a/chrome/installer/setup/uninstall.cc
+++ b/chrome/installer/setup/uninstall.cc
@@ -445,8 +445,13 @@ DeleteResult DeleteAppHostFilesAndFolders(const InstallerState& installer_state,
   if (!file_util::Delete(app_host_exe, false)) {
     result = DELETE_FAILED;
     LOG(ERROR) << "Failed to delete path: " << app_host_exe.value();
-  } else {
-    DeleteEmptyParentDir(target_path);
+  } else if (!target_path.empty() && file_util::IsDirectoryEmpty(target_path)) {

[gab, 2012/08/08 20:32:59]

I don't think target_path.empty() is possible at this point, remove this check.

[erikwright (departed), 2012/08/09 14:52:51]

It may be empty if both Chrome and App Host were installed in the same directory
but only App Host is uninstalled.

+    if (!file_util::Delete(target_path, true)) {
+      result = DELETE_FAILED;
+      LOG(ERROR) << "Failed to delete folder: " << target_path.value();
+    } else {
+      DeleteEmptyParentDir(target_path);
+    }
   }
 
   return result;
@@ -478,7 +483,7 @@ DeleteResult DeleteChromeFilesAndFolders(const InstallerState& installer_state,
     if (to_delete.BaseName().value() == installer::kChromeAppHostExe)
       continue;
 
-    VLOG(1) << "Deleting install path " << target_path.value();
+    VLOG(1) << "Deleting install path " << to_delete.value();
     if (!file_util::Delete(to_delete, true)) {
       LOG(ERROR) << "Failed to delete path (1st try): " << to_delete.value();
       if (installer_state.FindProduct(BrowserDistribution::CHROME_FRAME)) {
@@ -507,14 +512,21 @@ DeleteResult DeleteChromeFilesAndFolders(const InstallerState& installer_state,
   }
 
   if (result == DELETE_REQUIRES_REBOOT) {
+    // Delete the Application directory at reboot if empty.

[gab, 2012/08/08 20:32:59]

This function does not seem to wait for reboot for deletion (i.e. it doesn't
specify MOVEFILE_DELAY_UNTIL_REBOOT as per
http://msdn.microsoft.com/en-us/library/windows/desktop/aa365240(v=vs.85).aspx).

Please reword comment accordingly.

[erikwright (departed), 2012/08/09 14:52:51]

This is the implementation of ScheduleFileSystemEntityForDeletion:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/installer/util/dele...

It does specify that flag. I think the comment is then correct?

[gab, 2012/08/09 16:07:22]

Indeed :), guess I missed that line when reading the implemenation yesterday,
sorry about that!

+    ScheduleFileSystemEntityForDeletion(target_path.value().c_str());
     // If we need a reboot to continue, schedule the parent directories for

[gab, 2012/08/08 20:32:59]

nit: add empty line between code and comment

[erikwright (departed), 2012/08/09 14:52:51]

Done.

     // deletion unconditionally. If they are not empty, the session manager
     // will not delete them on reboot.
     ScheduleParentAndGrandparentForDeletion(target_path);
-  } else {
-    // Now check and delete if the parent directories are empty
-    // For example Google\Chrome or Chromium
-    DeleteEmptyParentDir(target_path);
+  } else if (file_util::IsDirectoryEmpty(target_path)) {
+    if (!file_util::Delete(target_path, true)) {
+      result = DELETE_FAILED;
+      LOG(ERROR) << "Failed to delete folder: " << target_path.value();

[gab, 2012/08/08 20:32:59]

Seems this log message is duplicated all over this file, I don't know if the
compiler is smart enough to consolidate those in a single data entry in the
binary... otherwise that's a lot of data for the same string...

Furthermore this logic is duplicated all over this file, does changing
DeleteEmptyParentDir() to something that does both make sense to you?

[erikwright (departed), 2012/08/09 14:52:51]

I thought about it, but it's also used in DeleteLocalState, where only two
directories are to be deleted.

String literals are folded during compilation.

Please see if the new patchset is more appealing.

+    } else {
+      // Now check and delete if the parent directories are empty
+      // For example Google\Chrome or Chromium
+      DeleteEmptyParentDir(target_path);
+    }
   }
   return result;
 }