Add WOW64 support and DeleteEmptyKey to base::win::registry.

base/win/registry.cc

Index: base/win/registry.cc
diff --git a/base/win/registry.cc b/base/win/registry.cc
index 7330d08aa277e0ab5a67d84492176556bdd9dde0..17969275f42bfc079acee2afb3d1ca39d0233b18 100644
--- a/base/win/registry.cc
+++ b/base/win/registry.cc
@@ -10,8 +10,7 @@
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/threading/thread_restrictions.h"
-
-#pragma comment(lib, "shlwapi.lib")  // for SHDeleteKey

[grt (UTC plus 2), 2014/05/16 15:44:43]

w00t!

[Will Harris, 2014/05/16 22:55:45]

:D

+#include "base/win/windows_version.h"
 
 namespace base {
 namespace win {
@@ -76,6 +75,7 @@ LONG RegKey::CreateWithDisposition(HKEY rootkey, const wchar_t* subkey,
   if (result == ERROR_SUCCESS) {
     Close();
     key_ = subhkey;
+    wow64access_ = access & KEY_WOW64_RES;

[grt (UTC plus 2), 2014/05/16 15:44:43]

looks like this is an undocumented constant in winnt.h. for safety's sake, wdyt
of making a constant in the unnamed namespace for this:

const REGSAM kWow64AccessMask = KEY_WOW64_32KEY | KEY_WOW64_64KEY;

[Will Harris, 2014/05/16 22:55:44]

Done.

   }
 
   return result;
@@ -86,10 +86,10 @@ LONG RegKey::CreateKey(const wchar_t* name, REGSAM access) {
   HKEY subkey = NULL;
   LONG result = RegCreateKeyEx(key_, name, 0, NULL, REG_OPTION_NON_VOLATILE,
                                access, NULL, &subkey, NULL);
- if (result == ERROR_SUCCESS) {
-   Close();
-
-   key_ = subkey;
+  if (result == ERROR_SUCCESS) {
+    Close();
+    key_ = subkey;
+    wow64access_ = access & KEY_WOW64_RES;
   }
 
   return result;
@@ -103,6 +103,7 @@ LONG RegKey::Open(HKEY rootkey, const wchar_t* subkey, REGSAM access) {
   if (result == ERROR_SUCCESS) {
     Close();
     key_ = subhkey;
+    wow64access_ = access & KEY_WOW64_RES;
   }
 
   return result;
@@ -118,6 +119,7 @@ LONG RegKey::OpenKey(const wchar_t* relative_key_name, REGSAM access) {
   if (result == ERROR_SUCCESS) {
     Close();
     key_ = subkey;
+    wow64access_ = access & KEY_WOW64_RES;
   }
   return result;
 }
@@ -168,8 +170,19 @@ LONG RegKey::GetValueNameAt(int index, std::wstring* name) const {
 LONG RegKey::DeleteKey(const wchar_t* name) {
   DCHECK(key_);
   DCHECK(name);
-  LONG result = SHDeleteKey(key_, name);
-  return result;
+  HKEY subkey;
+
+  // Verify the key exists before attempting delete to replicate previous
+  // behavior.
+  LONG result = RegOpenKeyEx(key_, name, 0, KEY_READ | wow64access_, &subkey);

[grt (UTC plus 2), 2014/05/16 15:44:43]

if this is just an existence test, i think READ_CONTROL is the minimal access
needed.

[Will Harris, 2014/05/16 22:55:44]

Done.

+  if (result == ERROR_SUCCESS) {

[grt (UTC plus 2), 2014/05/16 15:44:43]

suggestion:
  if (result != ERROR_SUCCESS)
    return result;
  ...

[Will Harris, 2014/05/16 22:55:44]

Done.

+    RegCloseKey(subkey);
+    std::wstring keyname(name);

[grt (UTC plus 2), 2014/05/16 15:44:43]

new code should use string16. would you mind switching
registry.{h,cc,_unittest.cc} wholesale in a separate change?

[Will Harris, 2014/05/16 22:55:44]

will do in another CL

+
+    return RegKey::RegDelRecurse(key_, keyname, wow64access_);

[grt (UTC plus 2), 2014/05/16 15:44:43]

remove RegKey::

[Will Harris, 2014/05/16 22:55:44]

Done.

+  } else {
+    return result;
+  }
 }
 
 LONG RegKey::DeleteValue(const wchar_t* value_name) {
@@ -342,6 +355,84 @@ LONG RegKey::StopWatching() {
   return result;
 }
 
+// static
+LONG RegKey::RegDeleteKeyExWrapper(HKEY hKey,

[grt (UTC plus 2), 2014/05/16 15:44:43]

use chromium naming style rather than hungarian

[Will Harris, 2014/05/16 22:55:44]

Was intentionally using these since it's a wrapper to an MS function.

+                                   const wchar_t* lpSubKey,
+                                   REGSAM samDesired,
+                                   DWORD Reserved) {
+  typedef LSTATUS(WINAPI *RegDeleteKeyExPtr)(
+      HKEY hKey, LPCWSTR lpSubKey, REGSAM samDesired, DWORD Reserved);
+
+  if (base::win::GetVersion() >= VERSION_VISTA) {

[grt (UTC plus 2), 2014/05/16 15:44:43]

some versions prior to vista have RegDeleteKeyEx, so i think it's better to
unconditionally try GetProcAddress and fallback to RegDeleteKey any time it
isn't found.

[Will Harris, 2014/05/16 22:55:44]

Done.  We need to support these Windows XP 64-bit users! :)

+    RegDeleteKeyExPtr reg_delete_key_ex_func =
+        reinterpret_cast<RegDeleteKeyExPtr>(
+            GetProcAddress(GetModuleHandleA("advapi32.dll"),
+                "RegDeleteKeyExW"));
+    if (reg_delete_key_ex_func)
+      return reg_delete_key_ex_func(hKey, lpSubKey, samDesired, Reserved);
+  } else {
+    // Windows XP does not support RegDeleteKeyEx.
+    return RegDeleteKey(hKey, lpSubKey);
+  }
+
+  return ERROR_GEN_FAILURE;
+}
+
+// static

[grt (UTC plus 2), 2014/05/16 15:44:43]

this isn't a static method according to the header.

[Will Harris, 2014/05/16 22:55:44]

Good spot - the static keyword was accidently dropped when I changed the
function prototype.

+LONG RegKey::RegDelRecurse(HKEY root_key, std::wstring name, REGSAM access) {

[grt (UTC plus 2), 2014/05/16 15:44:43]

pass name by const ref

[Will Harris, 2014/05/16 22:55:44]

Done.

+  LONG result;
+  DWORD key_size;
+  wchar_t key_name[MAX_PATH];
+  HKEY hKey;

[grt (UTC plus 2), 2014/05/16 15:44:43]

no hungarians allowed. i suggest target_key or something since this is a handle
to the key to be deleted.

[Will Harris, 2014/05/16 22:55:44]

Done.

+  FILETIME ft;

[grt (UTC plus 2), 2014/05/16 15:44:43]

is this needed? msdn says that the last param to EnumKeyEx can be NULL.

[Will Harris, 2014/05/16 22:55:44]

Done.

+
+  // First, see if the key can be deleted without having to recurse.
+  result = RegDeleteKeyExWrapper(root_key, name.c_str(), access, 0);
+
+  if (result == ERROR_SUCCESS)
+    return result;
+
+  result = RegOpenKeyEx(root_key, name.c_str(), 0, KEY_READ | access, &hKey);

[grt (UTC plus 2), 2014/05/16 15:44:43]

request the least required access:
KEY_READ -> KEY_ENUMERATE_SUB_KEYS

[Will Harris, 2014/05/16 22:55:45]

Done.

+
+  if (result != ERROR_SUCCESS) {

[grt (UTC plus 2), 2014/05/16 15:44:43]

  if (result == ERROR_FILE_NOT_FOUND)
    return ERROR_SUCCESS
  if (result != ERROR_SUCCESS)
    return result;

[Will Harris, 2014/05/16 22:55:44]

Done.

+    if (result == ERROR_FILE_NOT_FOUND) {
+      return ERROR_SUCCESS;
+    } else {
+      return result;
+    }
+  }
+
+  // Check for an ending slash and add one if it is missing.
+  if (name[name.length() - 1] != L'\\')

[grt (UTC plus 2), 2014/05/16 15:44:43]

what if |name| is empty? perhaps the top of the function should have:
  if (name.empty()) {
    NOTREACHED();
    return ERROR_INVALID_PARAMETER;
  }

[Will Harris, 2014/05/16 22:55:45]

Done.

+    name += L"\\";
+
+  // Enumerate the keys
+  key_size = MAX_PATH;
+  result = RegEnumKeyEx(hKey, 0, key_name, &key_size, NULL, NULL, NULL, &ft);

[grt (UTC plus 2), 2014/05/16 15:44:43]

i think cpu already mentioned this, but do something like this to have the key
name on the heap instead of the stack:
  const DWORD kMaxKeyNameLength = MAX_PATH;
  string16 key_name;
  DWORD key_name_length = kMaxKeyNameLength;
  result = RegEnumKeyEx(hKey, 0, WriteInto(&key_name, kMaxKeyNameLength),
                        &key_name_length, NULL, NULL, NULL, NULL);
  if (result == ERROR_SUCCESS)
    key_name.resize(key_name_length);

[Will Harris, 2014/05/16 22:55:45]

Done.

+
+  if (result == ERROR_SUCCESS) {

[grt (UTC plus 2), 2014/05/16 15:44:43]

can you get rid of if { do { with:
  for (; result == ERROR_SUCCESS; ) {

[Will Harris, 2014/05/16 22:55:44]

Done.

+    do {
+      std::wstring full_name(name);

[grt (UTC plus 2), 2014/05/16 15:44:43]

pull full_name's definition out of the loop

[Will Harris, 2014/05/16 22:55:44]

Done.

+
+      full_name += key_name;
+
+      if (RegDelRecurse(root_key, full_name, access) != ERROR_SUCCESS)
+        break;
+
+      key_size = MAX_PATH;
+      result = RegEnumKeyEx(hKey, 0, key_name, &key_size, NULL,
+          NULL, NULL, &ft);

[grt (UTC plus 2), 2014/05/16 15:44:43]

shouldn't this be aligned with hKey above? did "git cl format" format this?

[Will Harris, 2014/05/16 22:55:44]

Done.

+    } while (result == ERROR_SUCCESS);
+  }
+
+  RegCloseKey(hKey);
+
+  // Try again to delete the key.
+  result = RegDeleteKeyExWrapper(root_key, name.c_str(), access, 0);
+
+  return result;
+}
+
 // RegistryValueIterator ------------------------------------------------------
 
 RegistryValueIterator::RegistryValueIterator(HKEY root_key,