Delete both regular and Metro user data dirs on uninstall.

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 51b9717bcacda0bf8554dbbf2205172b18f0f84b..57c9c5b6b6bba14b88583a025a29e35e376869cb 100644
--- a/chrome/installer/setup/uninstall.cc
+++ b/chrome/installer/setup/uninstall.cc
@@ -339,23 +339,30 @@ bool DeleteEmptyParentDir(const FilePath& path) {
   return ret;
 }
 
-FilePath GetLocalStateFolder(const Product& product) {
+void GetLocalStateFolders(const Product& product,
+                          std::vector<FilePath>* paths) {
   // Obtain the location of the user profile data.
-  FilePath local_state_folder = product.GetUserDataPath();
-  LOG_IF(ERROR, local_state_folder.empty())
+  product.GetUserDataPaths(paths);
+  LOG_IF(ERROR, paths->empty())
       << "Could not retrieve user's profile directory.";
-
-  return local_state_folder;
 }
 
 // Creates a copy of the local state file and returns a path to the copy.
-FilePath BackupLocalStateFile(const FilePath& local_state_folder) {
+FilePath BackupLocalStateFile(
+    const std::vector<FilePath>& local_state_folders) {
   FilePath backup;
-  FilePath state_file(local_state_folder.Append(chrome::kLocalStateFilename));
-  if (!file_util::CreateTemporaryFile(&backup)) {
-    LOG(ERROR) << "Failed to create temporary file for Local State.";
-  } else {
-    file_util::CopyFile(state_file, backup);
+
+  // Copy the first local state file that is found.
+  for (size_t i = 0; i < local_state_folders.size(); ++i) {

[gab, 2012/08/03 17:54:20]

Is it ok if the Metro directory comes out of this? I'm not convinced from the
usage related to Chrome metrics in DoPostUninstallOperations().

[grt (UTC plus 2), 2012/08/03 20:18:37]

Yes.  Ideally, we'd aggregate the metrics from both sides, or at least pick the
more recent one, but something (desktop, falling back to metro) is better than
nothing in this case.

+    const FilePath& local_state_folder = local_state_folders[i];
+    FilePath state_file(local_state_folder.Append(chrome::kLocalStateFilename));
+    if (!file_util::PathExists(state_file))
+      continue;
+    if (!file_util::CreateTemporaryFile(&backup))
+      LOG(ERROR) << "Failed to create temporary file for Local State.";
+    else
+      file_util::CopyFile(state_file, backup);
+    break;
   }
   return backup;
 }
@@ -366,30 +373,35 @@ enum DeleteResult {
   DELETE_REQUIRES_REBOOT,
 };
 
-// Copies the local state to the temp folder and then deletes it.
-// The path to the copy is returned via the local_state_copy parameter.
-DeleteResult DeleteLocalState(const Product& product) {
-  FilePath user_local_state(GetLocalStateFolder(product));
-  if (user_local_state.empty())
+// Deletes all user data directories for a product.
+DeleteResult DeleteLocalState(const std::vector<FilePath>& local_state_folders,
+                              bool schedule_on_failure) {
+  if (local_state_folders.empty())
     return DELETE_SUCCEEDED;

[gab, 2012/08/03 17:54:20]

I think this should be: {
  NOTREACHED();
  return DELETE_FAILED;
}

[grt (UTC plus 2), 2012/08/03 20:18:37]

I added a DCHECK to GetChromeUserDataPaths() rather than sprinkling them around
the code.  Do you think that's sufficient?

[gab, 2012/08/03 20:42:41]

Yes, but I still think the return statement here should return DELETE_FAILED.

[grt (UTC plus 2), 2012/08/03 20:56:34]

hmm.  i disagree (although it's purely academic since the return value is
ignored).  if the vector was empty, the function was told to delete nothing at
all, which is a noop.  i don't think this should imply that the uninstall should
be aborted.

[gab, 2012/08/03 21:58:57]

Ah ok I see your point, given the returned value is not used though what about
making this function return void?

[grt (UTC plus 2), 2012/08/04 19:25:20]

After more thought, I like this as-is.  The fact that the (current) caller
doesn't care about the result doesn't mean that this function shouldn't have a
clean interface.

[gab, 2012/08/05 06:27:39]

sgtm

 
   DeleteResult result = DELETE_SUCCEEDED;
-  VLOG(1) << "Deleting user profile " << user_local_state.value();
-  if (!file_util::Delete(user_local_state, true)) {
-    LOG(ERROR) << "Failed to delete user profile dir: "
-               << user_local_state.value();
-    if (product.is_chrome_frame()) {
-      ScheduleDirectoryForDeletion(user_local_state.value().c_str());
-      result = DELETE_REQUIRES_REBOOT;
-    } else {
-      result = DELETE_FAILED;
+  const FilePath *user_local_state = NULL;

[gab, 2012/08/03 17:54:20]

Move this in the for loop (see comment below as to why you don't need it out of
that scope).

[grt (UTC plus 2), 2012/08/03 20:18:37]

Done.

+  for (size_t i = 0; i < local_state_folders.size(); ++i) {
+    user_local_state = &local_state_folders[i];
+    VLOG(1) << "Deleting user profile " << user_local_state->value();
+    if (!file_util::Delete(*user_local_state, true)) {
+      LOG(ERROR) << "Failed to delete user profile dir: "
+                 << user_local_state->value();
+      if (schedule_on_failure) {
+        ScheduleDirectoryForDeletion(user_local_state->value().c_str());
+        result = DELETE_REQUIRES_REBOOT;
+      } else {
+        result = DELETE_FAILED;
+      }
     }
   }
 
-  if (result == DELETE_REQUIRES_REBOOT) {
-    ScheduleParentAndGrandparentForDeletion(user_local_state);
-  } else {
-    DeleteEmptyParentDir(user_local_state);
+  if (user_local_state) {

[gab, 2012/08/03 17:54:20]

This check is unnecessary given you only proceed if !local_state_folders.empty()
above.

[grt (UTC plus 2), 2012/08/03 20:18:37]

Done.

+    if (result == DELETE_REQUIRES_REBOOT) {
+      ScheduleParentAndGrandparentForDeletion(*user_local_state);

[gab, 2012/08/03 17:54:20]

You can use local_state_folders[0] here instead (given your comment on
GetChromeUserDataPaths() that the first path is the primary one and that the
other ones are guaranteed to be siblings).

[grt (UTC plus 2), 2012/08/03 20:18:37]

Done.

+    } else {
+      DeleteEmptyParentDir(*user_local_state);
+    }
   }
 
   return result;
@@ -1101,8 +1113,9 @@ InstallStatus UninstallProduct(const InstallationState& original_state,
   // When deleting files, we must make sure that we're either a "single"
   // (aka non-multi) installation or we are the Chrome Binaries.
 
-  FilePath backup_state_file(
-      BackupLocalStateFile(GetLocalStateFolder(product)));
+  std::vector<FilePath> local_state_folders;
+  GetLocalStateFolders(product, &local_state_folders);
+  FilePath backup_state_file(BackupLocalStateFile(local_state_folders));
 
   DeleteResult delete_result = DELETE_SUCCEEDED;
 
@@ -1121,7 +1134,7 @@ InstallStatus UninstallProduct(const InstallationState& original_state,
   }
 
   if (delete_profile)
-    DeleteLocalState(product);
+    DeleteLocalState(local_state_folders, product.is_chrome_frame());
 
   if (delete_result == DELETE_FAILED) {
     ret = installer::UNINSTALL_FAILED;