Remove system-level Chrome ProgIDs from filetype associations at 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 0256590c65089166730ebc4df3528c0ad9af7b53..2e0a9fd59d4822389f76ebb839a62e57e45fbd2a 100644
--- a/chrome/installer/setup/uninstall.cc
+++ b/chrome/installer/setup/uninstall.cc
@@ -617,6 +617,76 @@ bool ShouldDeleteProfile(const InstallerState& installer_state,
   return should_delete;
 }
 
+// Removes XP-era filetype registration making Chrome the default browser.
+// MSDN (see http://msdn.microsoft.com/library/windows/desktop/cc144148.aspx)
+// tells us not to do this, but certain applications break following
+// uninstallation if we don't.
+void RemoveFiletypeRegistration(const InstallerState& installer_state,
+                                HKEY root,
+                                const string16& browser_entry_suffix) {
+  string16 classes_path(ShellUtil::kRegClasses);
+  classes_path.push_back(FilePath::kSeparators[0]);
+
+  const string16 prog_id(ShellUtil::kChromeHTMLProgId + browser_entry_suffix);
+
+  // Delete each filetype association if it references this Chrome.  Take care
+  // not to delete the association if it references a system-level install of
+  // Chrome (only a risk if the suffix is empty).  Don't delete the whole key
+  // since other apps may have stored data there.
+  std::vector<const wchar_t*> cleared_assocs;
+  if (installer_state.system_install() ||
+      !browser_entry_suffix.empty() ||
+      !base::win::RegKey(HKEY_LOCAL_MACHINE, (classes_path + prog_id).c_str(),
+                         KEY_QUERY_VALUE).Valid()) {
+    InstallUtil::ValueEquals prog_id_pred(prog_id);
+    for (const wchar_t* const* filetype = &ShellUtil::kFileAssociations[0];
+         *filetype != NULL; ++filetype) {
+      if (InstallUtil::DeleteRegistryValueIf(
+              root, (classes_path + *filetype).c_str(), L"",

[gab, 2012/09/25 14:58:32]

nit: Indent 1 more space here and below.

[grt (UTC plus 2), 2012/09/25 15:56:21]

it's currently indented four spaces from the 'I' above.  isn't this correct?

[gab, 2012/09/25 17:09:00]

I can't count...

+              prog_id_pred) == InstallUtil::DELETED) {
+        cleared_assocs.push_back(*filetype);

[gab, 2012/09/25 14:58:32]

Do we want to do the replacement in HKCU as well or only HKLM?

Seems to me in HKCU we can simply delete and let it fall back to HKLM default.

[grt (UTC plus 2), 2012/09/25 15:56:21]

only in HKLM.  in HKCU we delete Chrome's value but do not replace it.

[gab, 2012/09/25 17:09:00]

Right, hmmm well looks to me like this code populates |cleared_assocs| when
deleting values in HKCU as well... no?

In which case the code below swaps those values out in HKLM (which is actually
even worse...).

[grt (UTC plus 2), 2012/09/25 18:44:35]

Correct.
I don't think so.  The code below only does the replacing when the function is
operating in HKLM; see line 656.  Which one of us is missing something?! :-)

[gab, 2012/09/25 18:48:57]

Indeed, my bad, read passed the "if (root == HKEY_LOCAL_MACHINE)" line and then
the rest of the replacement block obviously has no further checks for this...

optional: What do you think of also not populating |cleared_assocs| for no
reason when operating in HKCU?

+      }
+    }
+  }
+
+  // For all filetype associations in HKLM that have just been removed, attempt
+  // to restore some reasonable value.  We have no definitive way of knowing
+  // what handlers are the most appropriate, so we use a fixed mapping based on
+  // the default values for a fresh install of Windows.
+  if (root == HKEY_LOCAL_MACHINE) {
+    string16 assoc;
+    base::win::RegKey key;
+
+    for (size_t i = 0; i < cleared_assocs.size(); ++i) {
+      const wchar_t* replacement_prog_id = NULL;
+      assoc.assign(cleared_assocs[i]);
+
+      // Inelegant, but simpler than a pure data-driven approach.
+      if (assoc == L".htm" || assoc == L".html")
+        replacement_prog_id = L"htmlfile";
+      else if (assoc == L".xht" || assoc == L".xhtml")
+        replacement_prog_id = L"xhtmlfile";
+
+      // If we have a replacement ProgID, see if it exists on this computer.
+      if (replacement_prog_id &&
+          key.Open(HKEY_LOCAL_MACHINE,
+                   (classes_path + replacement_prog_id).c_str(),
+                   KEY_QUERY_VALUE) == ERROR_SUCCESS) {
+        // Replace the absence of Chrome's ProgID with the Windows default.
+        if (key.Open(HKEY_LOCAL_MACHINE, (classes_path + assoc).c_str(),
+                     KEY_SET_VALUE) != ERROR_SUCCESS ||
+            key.WriteValue(NULL, replacement_prog_id) != ERROR_SUCCESS) {

[gab, 2012/09/25 14:58:32]

Here you use NULL and above (:645) you use L"" to represent the default value.

I think I prefer NULL, but either way please choose one and use it consistently
(maybe make a static const variable to indicate this is the Default value?... I
know it took me some time to figure out what NULL/L"" meant in both scenarios
(i.e. had to go lookup method definition in both cases)).

[grt (UTC plus 2), 2012/09/25 15:56:21]

addressing by fixing DeleteRegistryValueIf in a separate change, stay tuned.

+          LOG(ERROR) << "Failed to restore system-level filetype association "
+                     << assoc << " = " << replacement_prog_id;
+        }
+      } else if (replacement_prog_id) {

[gab, 2012/09/25 14:58:32]

I don't like to have to recheck this condition here, what about:

if (replacement_prog_id) {
  if (key.Open(...replacement_prog_id) == ERROR_SUCCESS &&
      (key.Open(...assoc) != ERROR_SUCCESS ||
       key.WriteValue(...) != ERROR_SUCCESS)) {
    LOG(ERROR) ...
  }
} else {
  LOG(WARNING) ...
}

[grt (UTC plus 2), 2012/09/25 15:56:21]

rejiggered.

+        LOG(WARNING) << "No known replacement ProgID for " << assoc
+                     << " files.";
+      }
+    }
+  }
+}
+
 bool DeleteChromeRegistrationKeys(const InstallerState& installer_state,
                                   BrowserDistribution* dist,
                                   HKEY root,
@@ -750,27 +820,7 @@ bool DeleteChromeRegistrationKeys(const InstallerState& installer_state,
                                      open_command_pred);
   }
 
-  // Delete each filetype association if it references this Chrome.  Take care
-  // not to delete the association if it references a system-level install of
-  // Chrome (only a risk if the suffix is empty).  Don't delete the whole key
-  // since other apps may have stored data there.
-  if (!browser_entry_suffix.empty() ||
-      (!installer_state.system_install() &&
-       !base::win::RegKey(HKEY_LOCAL_MACHINE, reg_prog_id.c_str(),
-                          KEY_QUERY_VALUE).Valid())) {
-    InstallUtil::ValueEquals prog_id_pred(prog_id);
-    for (const wchar_t* const* filetype = &ShellUtil::kFileAssociations[0];
-         *filetype != NULL; ++filetype) {
-      parent_key.resize(base_length);
-      parent_key.append(*filetype);
-      InstallUtil::DeleteRegistryValueIf(root, parent_key.c_str(), L"",
-                                         prog_id_pred);
-    }
-  }
-
-  // Note that we do not attempt to delete filetype associations since MSDN
-  // says "Windows respects the Default value only if the ProgID found there is
-  // a registered ProgID. If the ProgID is unregistered, it is ignored."
+  RemoveFiletypeRegistration(installer_state, root, browser_entry_suffix);
 
   *exit_code = installer::UNINSTALL_SUCCESSFUL;
   return true;