Regkey functions return error code instead of bool

Change the Regkey helper to consistently use and return LONG
instead of bool.  Fix RegKey usage all over the code base and
get rid of workarounds due to lack of return value.

Reviewers:

brettw: everything (skip parts for other reviewers if you wish)
robertshield,grt: chrome_frame, installer
siggi: ceee


BUG=none
TEST=covered by existing tests



Review URL: http://codereview.chromium.org/6090006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@71768 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 89 insertions, 84 deletions

diff --git a/chrome_frame/chrome_tab.cc b/chrome_frame/chrome_tab.cc
index 280e0a4..bc4a7d1 100644
--- a/chrome_frame/chrome_tab.cc
+++ b/chrome_frame/chrome_tab.cc
@@ -324,16 +324,16 @@ HRESULT SetupRunOnce() {
       }
 
       RegKey run_once;
-      if (run_once.Create(hive, kRunOnce, KEY_READ | KEY_WRITE)) {
+      LONG ret = run_once.Create(hive, kRunOnce, KEY_READ | KEY_WRITE);
+      if (ret == ERROR_SUCCESS) {
         CommandLine run_once_cmd(chrome_launcher::GetChromeExecutablePath());
         run_once_cmd.AppendSwitchASCII(switches::kAutomationClientChannelID,
                                        "0");
         run_once_cmd.AppendSwitch(switches::kChromeFrame);
-        if (run_once.WriteValue(L"A",
-                                run_once_cmd.command_line_string().c_str())) {
-          result = S_OK;
-        }
+        ret = run_once.WriteValue(L"A",
+                                  run_once_cmd.command_line_string().c_str());
       }
+      result = HRESULT_FROM_WIN32(ret);
     } else {
       result = S_FALSE;
     }
@@ -434,7 +434,8 @@ HRESULT SetChromeFrameUA(bool is_system, const wchar_t* value) {
   HKEY parent_hive = is_system ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER;
 
   RegKey ua_key;
-  if (ua_key.Create(parent_hive, kPostPlatformUAKey, KEY_READ | KEY_WRITE)) {
+  if (ua_key.Create(parent_hive, kPostPlatformUAKey,
+                    KEY_READ | KEY_WRITE) == ERROR_SUCCESS) {
     // Make sure that we unregister ChromeFrame UA strings registered previously
     wchar_t value_name[MAX_PATH + 1] = {};
     wchar_t value_data[MAX_PATH + 1] = {};
@@ -654,7 +655,7 @@ class SecurityDescBackup {
     RegKey backup_key(HKEY_LOCAL_MACHINE, backup_key_name_.c_str(),
                       KEY_READ | KEY_WRITE);
     if (backup_key.Valid()) {
-      return backup_key.WriteValue(NULL, str.GetString());
+      return backup_key.WriteValue(NULL, str.GetString()) == ERROR_SUCCESS;
     }
 
     return false;
@@ -696,15 +697,15 @@ class SecurityDescBackup {
 
     DWORD len = 0;
     DWORD reg_type = REG_NONE;
-    if (!backup_key.ReadValue(NULL, NULL, &len, &reg_type))
+    if (backup_key.ReadValue(NULL, NULL, &len, &reg_type) != ERROR_SUCCESS)
       return false;
 
     if (reg_type != REG_SZ)
       return false;
 
     size_t wchar_count = 1 + len / sizeof(wchar_t);
-    if (!backup_key.ReadValue(NULL, WriteInto(sddl, wchar_count), &len,
-                              &reg_type)) {
+    if (backup_key.ReadValue(NULL, WriteInto(sddl, wchar_count), &len,
+                             &reg_type) != ERROR_SUCCESS) {
       return false;
     }
 
@@ -760,16 +761,17 @@ static bool SetOrDeleteMimeHandlerKey(bool set, HKEY root_key) {
   if (!key.Valid())
     return false;
 
-  bool result;
+  LONG result1 = ERROR_SUCCESS;
+  LONG result2 = ERROR_SUCCESS;
   if (set) {
-    result = key.WriteValue(L"ChromeTab.ChromeActiveDocument", 1);
-    result = key.WriteValue(L"ChromeTab.ChromeActiveDocument.1", 1) && result;
+    result1 = key.WriteValue(L"ChromeTab.ChromeActiveDocument", 1);
+    result2 = key.WriteValue(L"ChromeTab.ChromeActiveDocument.1", 1);
   } else {
-    result = key.DeleteValue(L"ChromeTab.ChromeActiveDocument");
-    result = key.DeleteValue(L"ChromeTab.ChromeActiveDocument.1") && result;
+    result1 = key.DeleteValue(L"ChromeTab.ChromeActiveDocument");
+    result2 = key.DeleteValue(L"ChromeTab.ChromeActiveDocument.1");
   }
 
-  return result;
+  return (result2 == ERROR_SUCCESS) && (result2 == ERROR_SUCCESS);
 }
 
 bool RegisterSecuredMimeHandler(bool enable, bool is_system) {
diff --git a/chrome_frame/crash_reporting/crash_metrics.cc b/chrome_frame/crash_reporting/crash_metrics.cc
index 2759e5d..8909162 100644
--- a/chrome_frame/crash_reporting/crash_metrics.cc
+++ b/chrome_frame/crash_reporting/crash_metrics.cc
@@ -30,16 +30,19 @@ bool CrashMetricsReporter::SetMetric(Metric metric, int value) {
   DCHECK(metric >= NAVIGATION_COUNT && metric <= LAST_METRIC);
 
   base::win::RegKey metric_key;
-  if (metric_key.Create(HKEY_CURRENT_USER, kChromeFrameMetricsKey,
-                        KEY_SET_VALUE)) {
-    if (metric_key.WriteValue(g_metric_names[metric], value)) {
+  LONG result = metric_key.Create(HKEY_CURRENT_USER, kChromeFrameMetricsKey,
+                                  KEY_SET_VALUE);
+  if (result == ERROR_SUCCESS) {
+    result = metric_key.WriteValue(g_metric_names[metric], value);
+    if (result == ERROR_SUCCESS) {
       return true;
     } else {
       DLOG(ERROR) << "Failed to read ChromeFrame metric:"
-                  << g_metric_names[metric];
+        << g_metric_names[metric] << " error: " << result;
     }
   } else {
-    DLOG(ERROR) << "Failed to create ChromeFrame metrics key";
+    DLOG(ERROR) << "Failed to create ChromeFrame metrics key. error: "
+                << result;
   }
   return false;
 }
@@ -50,13 +53,11 @@ int CrashMetricsReporter::GetMetric(Metric metric) {
   int ret = 0;
   base::win::RegKey metric_key;
   if (metric_key.Open(HKEY_CURRENT_USER, kChromeFrameMetricsKey,
-                      KEY_QUERY_VALUE)) {
-    int value = 0;
-    if (metric_key.ReadValueDW(g_metric_names[metric],
-                               reinterpret_cast<DWORD*>(&value))) {
-      ret = value;
-    }
+                      KEY_QUERY_VALUE) == ERROR_SUCCESS) {
+    metric_key.ReadValueDW(g_metric_names[metric],
+                           reinterpret_cast<DWORD*>(&ret));
   }
+
   return ret;
 }
 
diff --git a/chrome_frame/policy_settings.cc b/chrome_frame/policy_settings.cc
index 50e465f..116052b 100644
--- a/chrome_frame/policy_settings.cc
+++ b/chrome_frame/policy_settings.cc
@@ -70,9 +70,11 @@ void PolicySettings::ReadUrlSettings(
   std::wstring settings_value(
       ASCIIToWide(policy::key::kChromeFrameRendererSettings));
   for (int i = 0; i < arraysize(kRootKeys); ++i) {
-    if (config_key.Open(kRootKeys[i], policy::kRegistrySubKey, KEY_READ) &&
-        config_key.ReadValueDW(settings_value.c_str(), &value)) {
-      break;
+    if ((config_key.Open(kRootKeys[i], policy::kRegistrySubKey,
+                         KEY_READ) == ERROR_SUCCESS) &&
+        (config_key.ReadValueDW(settings_value.c_str(),
+                                &value) == ERROR_SUCCESS)) {
+        break;
     }
   }
 
@@ -124,10 +126,11 @@ void PolicySettings::ReadApplicationLocaleSetting(
   std::wstring application_locale_value(
       ASCIIToWide(policy::key::kApplicationLocaleValue));
   for (int i = 0; i < arraysize(kRootKeys); ++i) {
-    if (config_key.Open(kRootKeys[i], policy::kRegistrySubKey, KEY_READ) &&
-        config_key.ReadValue(application_locale_value.c_str(),
-                             application_locale)) {
-      break;
+    if ((config_key.Open(kRootKeys[i], policy::kRegistrySubKey,
+                         KEY_READ) == ERROR_SUCCESS) &&
+        (config_key.ReadValue(application_locale_value.c_str(),
+                              application_locale) == ERROR_SUCCESS)) {
+        break;
     }
   }
 }
diff --git a/chrome_frame/ready_mode/internal/registry_ready_mode_state.cc b/chrome_frame/ready_mode/internal/registry_ready_mode_state.cc
index 244d63f5..880fd51 100644
--- a/chrome_frame/ready_mode/internal/registry_ready_mode_state.cc
+++ b/chrome_frame/ready_mode/internal/registry_ready_mode_state.cc
@@ -34,9 +34,11 @@ HANDLE LaunchCommandDirectly(const std::wstring& command_field) {
 
   for (int i = 0; i < arraysize(roots); i++) {
     base::win::RegKey version_key;
-    if (version_key.Open(roots[i], version_key_name.c_str(), KEY_QUERY_VALUE)) {
+    if (version_key.Open(roots[i], version_key_name.c_str(),
+                         KEY_QUERY_VALUE) == ERROR_SUCCESS) {
       std::wstring command_line;
-      if (version_key.ReadValue(command_field.c_str(), &command_line)) {
+      if (version_key.ReadValue(command_field.c_str(),
+                                &command_line) == ERROR_SUCCESS) {
         HANDLE launched_process = NULL;
         if (base::LaunchApp(command_line, false, true, &launched_process)) {
           return launched_process;
@@ -186,34 +188,24 @@ bool RegistryReadyModeState::GetValue(int64* value, bool* exists) {
   *exists = false;
   *value = 0;
 
-  HKEY roots[] = {HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE};
-
+  HKEY roots[] = { HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE };
+  LONG result = ERROR_SUCCESS;
   for (int i = 0; i < arraysize(roots); i++) {
     base::win::RegKey config_key;
-
-    if (config_key.Open(roots[i], key_name_.c_str(), KEY_QUERY_VALUE)) {
-      if (config_key.ValueExists(installer::kChromeFrameReadyModeField)) {
-        int64 temp;
-        DWORD value_size = sizeof(temp);
-        DWORD type = 0;
-        if (!config_key.ReadValue(installer::kChromeFrameReadyModeField,
-                                  &temp, &value_size, &type)) {
-          DLOG(ERROR) << "Failed to read from registry key " << key_name_
-                      << " and value " << installer::kChromeFrameReadyModeField;
-          return false;
-        }
-
-        if (value_size != sizeof(temp) || type != REG_QWORD) {
-          DLOG(ERROR) << "Unexpected state found under registry key "
-                      << key_name_ << " and value "
-                      << installer::kChromeFrameReadyModeField;
-          return false;
-        }
-
-        *value = temp;
+    result = config_key.Open(roots[i], key_name_.c_str(), KEY_QUERY_VALUE);
+    if (result == ERROR_SUCCESS) {
+      result = config_key.ReadInt64(installer::kChromeFrameReadyModeField,
+                                    value);
+      if (result == ERROR_SUCCESS) {
         *exists = true;
         return true;
       }
+      if (result != ERROR_FILE_NOT_FOUND) {
+        DLOG(ERROR) << "Failed to read from registry key " << key_name_
+                    << " and value " << installer::kChromeFrameReadyModeField
+                    << " error: " << result;
+        return false;
+      }
     }
   }
 
diff --git a/chrome_frame/test/chrome_frame_test_utils.cc b/chrome_frame/test/chrome_frame_test_utils.cc
index e464d6f..beeba70 100644
--- a/chrome_frame/test/chrome_frame_test_utils.cc
+++ b/chrome_frame/test/chrome_frame_test_utils.cc
@@ -633,8 +633,8 @@ TempRegKeyOverride::TempRegKeyOverride(HKEY override, const wchar_t* temp_name)
   DCHECK(temp_name && lstrlenW(temp_name));
   std::wstring key_path(kTempTestKeyPath);
   key_path += L"\\" + temp_name_;
-  EXPECT_TRUE(temp_key_.Create(HKEY_CURRENT_USER, key_path.c_str(),
-                               KEY_ALL_ACCESS));
+  EXPECT_EQ(ERROR_SUCCESS, temp_key_.Create(HKEY_CURRENT_USER, key_path.c_str(),
+                                            KEY_ALL_ACCESS));
   EXPECT_EQ(ERROR_SUCCESS,
             ::RegOverridePredefKey(override_, temp_key_.Handle()));
 }
@@ -647,7 +647,7 @@ TempRegKeyOverride::~TempRegKeyOverride() {
 // static
 void TempRegKeyOverride::DeleteAllTempKeys() {
   base::win::RegKey key;
-  if (key.Open(HKEY_CURRENT_USER, L"", KEY_ALL_ACCESS)) {
+  if (key.Open(HKEY_CURRENT_USER, L"", KEY_ALL_ACCESS) == ERROR_SUCCESS) {
     key.DeleteKey(kTempTestKeyPath);
   }
 }
diff --git a/chrome_frame/test/perf/chrome_frame_perftest.cc b/chrome_frame/test/perf/chrome_frame_perftest.cc
index e59c843..2f645d2 100644
--- a/chrome_frame/test/perf/chrome_frame_perftest.cc
+++ b/chrome_frame/test/perf/chrome_frame_perftest.cc
@@ -1105,7 +1105,7 @@ TEST_F(FlashCreationTest, PerfCold) {
   base::win::RegKey flash_key(HKEY_CLASSES_ROOT, kFlashControlKey, KEY_READ);
 
   std::wstring plugin_path;
-  ASSERT_TRUE(flash_key.ReadValue(L"", &plugin_path));
+  ASSERT_EQ(ERROR_SUCCESS, flash_key.ReadValue(L"", &plugin_path));
   ASSERT_FALSE(plugin_path.empty());
 
   FilePath flash_path = FilePath::FromWStringHack(plugin_path);
@@ -1126,7 +1126,7 @@ TEST_F(SilverlightCreationTest, DISABLED_PerfCold) {
                                     KEY_READ);
 
   std::wstring plugin_path;
-  ASSERT_TRUE(silverlight_key.ReadValue(L"", &plugin_path));
+  ASSERT_EQ(ERROR_SUCCESS, silverlight_key.ReadValue(L"", &plugin_path));
   ASSERT_FALSE(plugin_path.empty());
 
   FilePath silverlight_path = FilePath::FromWStringHack(plugin_path);
diff --git a/chrome_frame/test/policy_settings_unittest.cc b/chrome_frame/test/policy_settings_unittest.cc
index 771762f..c5d7c50 100644
--- a/chrome_frame/test/policy_settings_unittest.cc
+++ b/chrome_frame/test/policy_settings_unittest.cc
@@ -23,7 +23,8 @@ namespace {
 // A best effort way to zap CF policy entries that may be in the registry.
 void DeleteChromeFramePolicyEntries(HKEY root) {
   RegKey key;
-  if (key.Open(root, policy::kRegistrySubKey, KEY_ALL_ACCESS)) {
+  if (key.Open(root, policy::kRegistrySubKey,
+               KEY_ALL_ACCESS) == ERROR_SUCCESS) {
     key.DeleteValue(
         ASCIIToWide(policy::key::kChromeFrameRendererSettings).c_str());
     key.DeleteKey(ASCIIToWide(policy::key::kRenderInChromeFrameList).c_str());
@@ -34,8 +35,8 @@ void DeleteChromeFramePolicyEntries(HKEY root) {
 }
 
 bool InitializePolicyKey(HKEY policy_root, RegKey* policy_key) {
-  EXPECT_TRUE(policy_key->Create(policy_root, policy::kRegistrySubKey,
-                                 KEY_ALL_ACCESS));
+  EXPECT_EQ(ERROR_SUCCESS, policy_key->Create(policy_root,
+      policy::kRegistrySubKey, KEY_ALL_ACCESS));
   return policy_key->Valid();
 }
 
@@ -46,10 +47,11 @@ void WritePolicyList(RegKey* policy_key, const wchar_t* list_name,
   policy_key->DeleteKey(list_name);
 
   RegKey list_key;
-  EXPECT_TRUE(list_key.Create(policy_key->Handle(), list_name, KEY_ALL_ACCESS));
+  EXPECT_EQ(ERROR_SUCCESS, list_key.Create(policy_key->Handle(), list_name,
+                                           KEY_ALL_ACCESS));
   for (int i = 0; i < count; ++i) {
-    EXPECT_TRUE(list_key.WriteValue(base::StringPrintf(L"%i", i).c_str(),
-                                    values[i]));
+    EXPECT_EQ(ERROR_SUCCESS,
+        list_key.WriteValue(base::StringPrintf(L"%i", i).c_str(), values[i]));
   }
 }
 
@@ -94,7 +96,8 @@ bool SetChromeApplicationLocale(HKEY policy_root, const wchar_t* locale) {
 
   std::wstring application_locale_value(
       ASCIIToWide(policy::key::kApplicationLocaleValue));
-  EXPECT_TRUE(policy_key.WriteValue(application_locale_value.c_str(), locale));
+  EXPECT_EQ(ERROR_SUCCESS,
+      policy_key.WriteValue(application_locale_value.c_str(), locale));
   return true;
 }
 
diff --git a/chrome_frame/utils.cc b/chrome_frame/utils.cc
index c23080c..1445ccb 100644
--- a/chrome_frame/utils.cc
+++ b/chrome_frame/utils.cc
@@ -233,7 +233,8 @@ bool UtilChangePersistentNPAPIMarker(bool set) {
   bool success = false;
   if (cf_state_key.Valid()) {
     if (set) {
-      success = cf_state_key.WriteValue(kChromeFramePersistNPAPIReg, 1);
+      success = (cf_state_key.WriteValue(kChromeFramePersistNPAPIReg, 1) ==
+          ERROR_SUCCESS);
     } else {
       // Unfortunately, DeleteValue returns true only if the value
       // previously existed, so we do a separate existence check to
@@ -255,7 +256,8 @@ bool UtilIsPersistentNPAPIMarkerSet() {
   bool success = false;
   if (cf_state_key.Valid()) {
     DWORD val = 0;
-    if (cf_state_key.ReadValueDW(kChromeFramePersistNPAPIReg, &val)) {
+    if (cf_state_key.ReadValueDW(kChromeFramePersistNPAPIReg, &val) ==
+        ERROR_SUCCESS) {
       success = (val != 0);
     }
   }
@@ -676,11 +678,8 @@ int GetConfigInt(int default_value, const wchar_t* value_name) {
   int ret = default_value;
   RegKey config_key;
   if (config_key.Open(HKEY_CURRENT_USER, kChromeFrameConfigKey,
-                      KEY_QUERY_VALUE)) {
-    int value = FALSE;
-    if (config_key.ReadValueDW(value_name, reinterpret_cast<DWORD*>(&value))) {
-      ret = value;
-    }
+                      KEY_QUERY_VALUE) == ERROR_SUCCESS) {
+    config_key.ReadValueDW(value_name, reinterpret_cast<DWORD*>(&ret));
   }
 
   return ret;
@@ -694,8 +693,8 @@ bool GetConfigBool(bool default_value, const wchar_t* value_name) {
 bool SetConfigInt(const wchar_t* value_name, int value) {
   RegKey config_key;
   if (config_key.Create(HKEY_CURRENT_USER, kChromeFrameConfigKey,
-                        KEY_SET_VALUE)) {
-    if (config_key.WriteValue(value_name, value)) {
+                        KEY_SET_VALUE) == ERROR_SUCCESS) {
+    if (config_key.WriteValue(value_name, value) == ERROR_SUCCESS) {
       return true;
     }
   }
@@ -710,8 +709,10 @@ bool SetConfigBool(const wchar_t* value_name, bool value) {
 bool DeleteConfigValue(const wchar_t* value_name) {
   RegKey config_key;
   if (config_key.Open(HKEY_CURRENT_USER, kChromeFrameConfigKey,
-                      KEY_WRITE)) {
-    return config_key.DeleteValue(value_name);
+                      KEY_WRITE) == ERROR_SUCCESS) {
+    if (config_key.DeleteValue(value_name) == ERROR_SUCCESS) {
+      return true;
+    }
   }
   return false;
 }
@@ -728,7 +729,8 @@ bool IsGcfDefaultRenderer() {
     // TODO(tommi): Implement caching for this config value as it gets
     // checked frequently.
     RegKey config_key;
-    if (config_key.Open(HKEY_CURRENT_USER, kChromeFrameConfigKey, KEY_READ)) {
+    if (config_key.Open(HKEY_CURRENT_USER, kChromeFrameConfigKey,
+                        KEY_READ) == ERROR_SUCCESS) {
       config_key.ReadValueDW(kEnableGCFRendererByDefault, &is_default);
     }
   }
@@ -751,8 +753,10 @@ RendererType RendererTypeForUrl(const std::wstring& url) {
   }
 
   RegKey config_key;
-  if (!config_key.Open(HKEY_CURRENT_USER, kChromeFrameConfigKey, KEY_READ))
+  if (config_key.Open(HKEY_CURRENT_USER, kChromeFrameConfigKey,
+                      KEY_READ) != ERROR_SUCCESS) {
     return RENDERER_TYPE_UNDETERMINED;
+  }
 
   RendererType renderer_type = RENDERER_TYPE_UNDETERMINED;