Delete the installation folder after deleting its contents.

chrome/installer/setup/uninstall.cc

 // Copyright (c) 2012 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.
 //
 // This file defines the methods useful for uninstalling Chrome.
 
 #include "chrome/installer/setup/uninstall.h"
 
 #include <windows.h>
 
@@ skipping 427 matching lines @@
   }
 
   DeleteInstallTempDir(target_path);
 
   DeleteResult result = DELETE_SUCCEEDED;
 
   FilePath app_host_exe(target_path.Append(installer::kChromeAppHostExe));
   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;
 }
 
 DeleteResult DeleteChromeFilesAndFolders(const InstallerState& installer_state,
                                          const Version& installed_version) {
   const FilePath& target_path = installer_state.target_path();
   if (target_path.empty()) {
     LOG(ERROR) << "DeleteChromeFilesAndFolders: no installation destination "
@@ skipping 11 matching lines @@
       false,
       static_cast<FileEnumerator::FileType>(FileEnumerator::FILES |
                                             FileEnumerator::DIRECTORIES));
   while (true) {
     FilePath to_delete(file_enumerator.Next());
     if (to_delete.empty())
       break;
     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)) {
         // We don't try killing Chrome processes for Chrome Frame builds since
         // that is unlikely to help. Instead, schedule files for deletion and
         // return a value that will trigger a reboot prompt.
         FileEnumerator::FindInfo find_info;
         file_enumerator.GetFindInfo(&find_info);
         if (FileEnumerator::IsDirectory(find_info))
           ScheduleDirectoryForDeletion(to_delete.value().c_str());
         else
           ScheduleFileSystemEntityForDeletion(to_delete.value().c_str());
         result = DELETE_REQUIRES_REBOOT;
       } else {
         // Try closing any running Chrome processes and deleting files once
         // again.
         CloseAllChromeProcesses();
         if (!file_util::Delete(to_delete, true)) {
           LOG(ERROR) << "Failed to delete path (2nd try): "
                      << to_delete.value();
           result = DELETE_FAILED;
           break;
         }
       }
     }
   }
 
   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;
 }
 
 // This method checks if Chrome is currently running or if the user has
 // cancelled the uninstall operation by clicking Cancel on the confirmation
 // box that Chrome pops up.
 InstallStatus IsChromeActiveOrUserCancelled(
     const InstallerState& installer_state,
     const Product& product) {
@@ skipping 633 matching lines @@
 
   // Try and delete the preserved local state once the post-install
   // operations are complete.
   if (!backup_state_file.empty())
     file_util::Delete(backup_state_file, false);
 
   return ret;
 }
 
 }  // namespace installer