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..07e21b588048560c9456935e14744886e87fe3ae 100644
--- a/chrome/installer/setup/uninstall.cc
+++ b/chrome/installer/setup/uninstall.cc
@@ -318,25 +318,28 @@ bool ScheduleParentAndGrandparentForDeletion(const FilePath& path) {
   return ret;
 }
 
-// Deletes empty parent & empty grandparent dir of given path.
-bool DeleteEmptyParentDir(const FilePath& path) {
-  bool ret = true;
-  FilePath parent_dir = path.DirName();
-  if (!parent_dir.empty() && file_util::IsDirectoryEmpty(parent_dir)) {
-    if (!file_util::Delete(parent_dir, true)) {
-      ret = false;
-      LOG(ERROR) << "Failed to delete folder: " << parent_dir.value();
-    }
+enum DeleteResult {
+  DELETE_SUCCEEDED,
+  DELETE_NOT_EMPTY,
+  DELETE_FAILED,
+  DELETE_REQUIRES_REBOOT,
+};
 
-    parent_dir = parent_dir.DirName();
-    if (!parent_dir.empty() && file_util::IsDirectoryEmpty(parent_dir)) {
-      if (!file_util::Delete(parent_dir, true)) {
-        ret = false;
-        LOG(ERROR) << "Failed to delete folder: " << parent_dir.value();
-      }
-    }
-  }
-  return ret;
+// Deletes the given directory if it is empty. Returns DELETE_SUCCEEDED if the
+// directory is deleted, DELETE_NOT_EMPTY if it is not empty, and DELETE_FAILED
+// otherwise.
+DeleteResult DeleteEmptyDir(const FilePath& path) {
+  if (path.empty())
+    return DELETE_FAILED;
+
+  if (!file_util::IsDirectoryEmpty(path))
+    return DELETE_NOT_EMPTY;
+
+  if (file_util::Delete(path, true))
+    return DELETE_SUCCEEDED;
+
+  LOG(ERROR) << "Failed to delete folder: " << path.value();
+  return DELETE_FAILED;
 }
 
 void GetLocalStateFolders(const Product& product,
@@ -367,12 +370,6 @@ FilePath BackupLocalStateFile(
   return backup;
 }
 
-enum DeleteResult {
-  DELETE_SUCCEEDED,
-  DELETE_FAILED,
-  DELETE_REQUIRES_REBOOT,
-};
-
 // Deletes all user data directories for a product.
 DeleteResult DeleteLocalState(const std::vector<FilePath>& local_state_folders,
                               bool schedule_on_failure) {
@@ -398,7 +395,12 @@ DeleteResult DeleteLocalState(const std::vector<FilePath>& local_state_folders,
   if (result == DELETE_REQUIRES_REBOOT) {
     ScheduleParentAndGrandparentForDeletion(local_state_folders[0]);
   } else {
-    DeleteEmptyParentDir(local_state_folders[0]);
+    FilePath user_data_dir = local_state_folders[0].DirName();

[gab, 2012/08/13 18:20:15]

nit: As per Chromium style, initialize non-POD types using '()' rather than '='.

(and make it 'const')

[erikwright (departed), 2012/08/13 19:27:36]

Done.

+    if (!user_data_dir.empty() &&

[gab, 2012/08/13 18:20:15]

why do you need to first check if it isn't empty? (and even if you do I feel the
current conditional is wrong and should be the opposite, i.e. check that it IS
empty... no?)

[erikwright (departed), 2012/08/13 19:27:36]

Note that this is querying the path string and not the filesystem.

+        DELETE_SUCCEEDED == DeleteEmptyDir(user_data_dir)) {

[gab, 2012/08/13 18:20:15]

nit: put constant on RHS of ==

[erikwright (departed), 2012/08/13 19:27:36]

Done.

+      FilePath product_dir = user_data_dir.DirName();

[gab, 2012/08/13 18:20:15]

nit: initialization style as above

optional: I would personally prefer to have this inlined below.

[erikwright (departed), 2012/08/13 19:27:36]

Done.
I think labeling it has merit. Also, now it's used twice.

+      DeleteEmptyDir(product_dir);
+    }
   }
 
   return result;
@@ -428,6 +430,26 @@ bool MoveSetupOutOfInstallFolder(const InstallerState& installer_state,
   return ret;
 }
 
+DeleteResult DeleteApplicationProductAndVendorDirectories(

[gab, 2012/08/13 18:20:15]

Wouldn't it make more sense to simply delete all parents in the hierarchy
iteratively until one of them is not empty?

Or take a number of parent directories to delete and expect to be able to delete
that amount iteratively (or return the number deleted and make that check caller
side)?

[erikwright (departed), 2012/08/13 19:27:36]

No. It seems wrong to attempt to delete AppData or AppData/Local even if we do
first check that they're empty.
Seems better to me to explicitly state in code what directories we are
attempting to delete, rather than having mysterious literals like '2' and '3'.

+    const FilePath& application_directory) {
+  DeleteResult result = DeleteEmptyDir(application_directory);
+  if (result == DELETE_SUCCEEDED) {
+    // Now check and delete if the parent directories are empty
+    // For example Google\Chrome or Chromium
+    FilePath product_directory = application_directory.DirName();

[gab, 2012/08/13 18:20:15]

nit: initialization style as above

[erikwright (departed), 2012/08/13 19:27:36]

Done.

+    if (!product_directory.empty()) {

[gab, 2012/08/13 18:20:15]

As above I don't see the need for this check and feel it is backwards if
anything...

[erikwright (departed), 2012/08/13 19:27:36]

::empty() is querying the path string, not the filesystem.

+        result = DeleteEmptyDir(product_directory);
+        if (result == DELETE_SUCCEEDED) {
+          FilePath vendor_directory = product_directory.DirName();
+          result = DeleteEmptyDir(vendor_directory);
+        }
+    }
+  }
+  if (result == DELETE_NOT_EMPTY)
+    result = DELETE_SUCCEEDED;
+  return result;
+}
+
 DeleteResult DeleteAppHostFilesAndFolders(const InstallerState& installer_state,
                                           const Version& installed_version) {
   const FilePath& target_path = installer_state.target_path();
@@ -446,7 +468,7 @@ DeleteResult DeleteAppHostFilesAndFolders(const InstallerState& installer_state,
     result = DELETE_FAILED;
     LOG(ERROR) << "Failed to delete path: " << app_host_exe.value();
   } else {
-    DeleteEmptyParentDir(target_path);
+    result = DeleteApplicationProductAndVendorDirectories(target_path);
   }
 
   return result;
@@ -478,7 +500,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 +529,15 @@ DeleteResult DeleteChromeFilesAndFolders(const InstallerState& installer_state,
   }
 
   if (result == DELETE_REQUIRES_REBOOT) {
+    // Delete the Application directory at reboot if empty.
+    ScheduleFileSystemEntityForDeletion(target_path.value().c_str());
+
     // If we need a reboot to continue, schedule the parent directories for
     // 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);
+    result = DeleteApplicationProductAndVendorDirectories(target_path);
   }
   return result;
 }