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, 91 insertions, 68 deletions

diff --git a/ceee/ie/common/ceee_module_util.cc b/ceee/ie/common/ceee_module_util.cc
index 836f031..e3d10eb 100644
--- a/ceee/ie/common/ceee_module_util.cc
+++ b/ceee/ie/common/ceee_module_util.cc
@@ -43,16 +43,9 @@ bool GetCeeeRegistryBoolean(const wchar_t* key, bool default_value) {
   base::win::RegKey hkcu(HKEY_CURRENT_USER, kRegistryPath, KEY_READ);
   LOG_IF(ERROR, !hkcu.Valid()) << "Could not open reg key: " << kRegistryPath;
 
-  DWORD dword_value_representation = 0;
-  DWORD size = sizeof(dword_value_representation);
-  DWORD type = REG_DWORD;
-
-  if (!hkcu.Valid() ||
-      !hkcu.ReadValue(key, &dword_value_representation, &size, &type)) {
-    return default_value;
-  }
-
-  return dword_value_representation != 0;
+  DWORD value = default_value ? 1 : 0;
+  hkcu.ReadValueDW(key, &value);
+  return value != 0;
 }
 
 void SetCeeeRegistryBoolean(const wchar_t* key, bool assign_value) {
@@ -60,11 +53,13 @@ void SetCeeeRegistryBoolean(const wchar_t* key, bool assign_value) {
   LOG_IF(ERROR, !hkcu.Valid()) << "Could not open reg key: " << kRegistryPath;
 
   DWORD dword_value_representation = assign_value ? 1 : 0;
-  bool write_result = hkcu.WriteValue(key, &dword_value_representation,
+  LONG write_result = hkcu.WriteValue(key, &dword_value_representation,
                                       sizeof(dword_value_representation),
                                       REG_DWORD);
 
-  LOG_IF(ERROR, !write_result) << "Failed to write a registry key: " << key;
+  LOG_IF(ERROR, write_result != ERROR_SUCCESS)
+      << "Failed to write a registry key: " << key
+      << " error: " << com::LogWe(write_result);
 }
 
 }  // anonymous namespace
@@ -111,8 +106,10 @@ std::wstring GetExtensionPath() {
 
   base::win::RegKey* keys[] = { &hkcu, &hklm };
   for (int i = 0; i < arraysize(keys); ++i) {
-    if (keys[i]->Valid() && keys[i]->ReadValue(kRegistryValue, &crx_path))
+    if (keys[i]->Valid() &&
+        (keys[i]->ReadValue(kRegistryValue, &crx_path) == ERROR_SUCCESS)) {
       break;
+    }
   }
 
   if (crx_path.size() == 0u) {
@@ -196,8 +193,8 @@ bool NeedToInstallExtension() {
         // we might need to retry installation.
         std::wstring version_string;
         base::win::RegKey hkcu(HKEY_CURRENT_USER, kRegistryPath, KEY_READ);
-        success = hkcu.ReadValue(
-            kRegistryValueCrxInstalledByVersion, &version_string);
+        success = hkcu.ReadValue(kRegistryValueCrxInstalledByVersion,
+                                 &version_string) == ERROR_SUCCESS;
         return !success || version_string != TO_L_STRING(CHROME_VERSION_STRING);
       }
     }
@@ -216,17 +213,18 @@ void SetInstalledExtensionPath(const FilePath& path) {
       base::Time::Now().ToInternalValue();
 
   base::win::RegKey key(HKEY_CURRENT_USER, kRegistryPath, KEY_WRITE);
-  bool write_result = key.WriteValue(kRegistryValueCrxInstalledTime,
+  LONG write_result = key.WriteValue(kRegistryValueCrxInstalledTime,
                                      &crx_time,
                                      sizeof(crx_time),
                                      REG_QWORD);
-  DCHECK(write_result);
+  DCHECK_EQ(ERROR_SUCCESS, write_result);
   write_result = key.WriteValue(kRegistryValueCrxInstalledPath,
                                 path.value().c_str());
-  DCHECK(write_result);
+  DCHECK_EQ(ERROR_SUCCESS, write_result);
 
   write_result = key.WriteValue(kRegistryValueCrxInstalledByVersion,
                                 TO_L_STRING(CHROME_VERSION_STRING));
+  DCHECK_EQ(ERROR_SUCCESS, write_result);
 }
 
 bool IsCrxOrEmpty(const std::wstring& path) {
@@ -301,12 +299,13 @@ std::wstring GetCromeFrameClientStateKey() {
 bool GetCollectStatsConsent() {
   std::wstring reg_path = GetCromeFrameClientStateKey();
   base::win::RegKey key(HKEY_CURRENT_USER, reg_path.c_str(), KEY_READ);
-  DWORD value;
-  if (!key.ReadValueDW(google_update::kRegUsageStatsField, &value)) {
+  DWORD value = 0;
+  if (key.ReadValueDW(google_update::kRegUsageStatsField, &value) !=
+      ERROR_SUCCESS) {
     base::win::RegKey hklm_key(HKEY_LOCAL_MACHINE, reg_path.c_str(), KEY_READ);
-    if (!hklm_key.ReadValueDW(google_update::kRegUsageStatsField, &value))
-      return false;
+    key.ReadValueDW(google_update::kRegUsageStatsField, &value);
   }
+
   return (1 == value);
 }
 
@@ -326,7 +325,7 @@ bool RefreshElevationPolicyIfNeeded() {
 
   static const wchar_t kValueName[] = L"last_elevation_refresh";
   std::wstring last_elevation_refresh_version;
-  bool result = hkcu.ReadValue(kValueName, &last_elevation_refresh_version);
+  LONG result = hkcu.ReadValue(kValueName, &last_elevation_refresh_version);
   if (last_elevation_refresh_version == expected_version)
     return false;
 
@@ -336,7 +335,8 @@ bool RefreshElevationPolicyIfNeeded() {
   // Write after refreshing because it's better to refresh twice, than to miss
   // once.
   result = hkcu.WriteValue(kValueName, expected_version.c_str());
-  LOG_IF(ERROR, !result) << "Failed to write a registry value: " << kValueName;
+  LOG_IF(ERROR, result != ERROR_SUCCESS) << "Failed to write a registry value: "
+      << kValueName << " error: " << com::LogWe(result);
 
   return true;
 }
diff --git a/ceee/ie/common/ceee_module_util_unittest.cc b/ceee/ie/common/ceee_module_util_unittest.cc
index 5bda65d..49352c5 100644
--- a/ceee/ie/common/ceee_module_util_unittest.cc
+++ b/ceee/ie/common/ceee_module_util_unittest.cc
@@ -207,16 +207,20 @@ TEST_F(CeeeModuleUtilTest, GetCollectStatsConsentFromHkcu) {
   base::win::RegKey hkcu(HKEY_CURRENT_USER,
       ceee_module_util::GetCromeFrameClientStateKey().c_str(), KEY_WRITE);
 
-  ASSERT_TRUE(hkcu.WriteValue(google_update::kRegUsageStatsField, kTrue));
+  ASSERT_EQ(ERROR_SUCCESS,
+      hkcu.WriteValue(google_update::kRegUsageStatsField, kTrue));
   EXPECT_TRUE(ceee_module_util::GetCollectStatsConsent());
 
-  ASSERT_TRUE(hkcu.WriteValue(google_update::kRegUsageStatsField, kFalse));
+  ASSERT_EQ(ERROR_SUCCESS,
+      hkcu.WriteValue(google_update::kRegUsageStatsField, kFalse));
   EXPECT_FALSE(ceee_module_util::GetCollectStatsConsent());
 
-  ASSERT_TRUE(hkcu.WriteValue(google_update::kRegUsageStatsField, kInvalid));
+  ASSERT_EQ(ERROR_SUCCESS,
+      hkcu.WriteValue(google_update::kRegUsageStatsField, kInvalid));
   EXPECT_FALSE(ceee_module_util::GetCollectStatsConsent());
 
-  ASSERT_TRUE(hkcu.DeleteValue(google_update::kRegUsageStatsField));
+  ASSERT_EQ(ERROR_SUCCESS,
+      hkcu.DeleteValue(google_update::kRegUsageStatsField));
   EXPECT_FALSE(ceee_module_util::GetCollectStatsConsent());
 }
 
@@ -225,16 +229,20 @@ TEST_F(CeeeModuleUtilTest, GetCollectStatsConsentFromHklm) {
   base::win::RegKey hklm(HKEY_LOCAL_MACHINE,
       ceee_module_util::GetCromeFrameClientStateKey().c_str(), KEY_WRITE);
 
-  ASSERT_TRUE(hklm.WriteValue(google_update::kRegUsageStatsField, kTrue));
+  ASSERT_EQ(ERROR_SUCCESS,
+      hklm.WriteValue(google_update::kRegUsageStatsField, kTrue));
   EXPECT_TRUE(ceee_module_util::GetCollectStatsConsent());
 
-  ASSERT_TRUE(hklm.WriteValue(google_update::kRegUsageStatsField, kFalse));
+  ASSERT_EQ(ERROR_SUCCESS,
+      hklm.WriteValue(google_update::kRegUsageStatsField, kFalse));
   EXPECT_FALSE(ceee_module_util::GetCollectStatsConsent());
 
-  ASSERT_TRUE(hklm.WriteValue(google_update::kRegUsageStatsField, kInvalid));
+  ASSERT_EQ(ERROR_SUCCESS,
+      hklm.WriteValue(google_update::kRegUsageStatsField, kInvalid));
   EXPECT_FALSE(ceee_module_util::GetCollectStatsConsent());
 
-  ASSERT_TRUE(hklm.DeleteValue(google_update::kRegUsageStatsField));
+  ASSERT_EQ(ERROR_SUCCESS,
+      hklm.DeleteValue(google_update::kRegUsageStatsField));
   EXPECT_FALSE(ceee_module_util::GetCollectStatsConsent());
 }
 
@@ -246,14 +254,18 @@ TEST_F(CeeeModuleUtilTest, GetCollectStatsConsentHkcuBeforeHklm) {
   base::win::RegKey hklm(HKEY_LOCAL_MACHINE,
       ceee_module_util::GetCromeFrameClientStateKey().c_str(), KEY_WRITE);
 
-  ASSERT_TRUE(hklm.WriteValue(google_update::kRegUsageStatsField, kTrue));
+  ASSERT_EQ(ERROR_SUCCESS,
+      hklm.WriteValue(google_update::kRegUsageStatsField, kTrue));
   ASSERT_TRUE(ceee_module_util::GetCollectStatsConsent());
 
-  ASSERT_TRUE(hkcu.WriteValue(google_update::kRegUsageStatsField, kFalse));
+  ASSERT_EQ(ERROR_SUCCESS,
+      hkcu.WriteValue(google_update::kRegUsageStatsField, kFalse));
   EXPECT_FALSE(ceee_module_util::GetCollectStatsConsent());
 
-  ASSERT_TRUE(hkcu.DeleteValue(google_update::kRegUsageStatsField));
-  ASSERT_TRUE(hklm.DeleteValue(google_update::kRegUsageStatsField));
+  ASSERT_EQ(ERROR_SUCCESS,
+      hkcu.DeleteValue(google_update::kRegUsageStatsField));
+  ASSERT_EQ(ERROR_SUCCESS,
+      hklm.DeleteValue(google_update::kRegUsageStatsField));
 }
 
 }  // namespace
diff --git a/ceee/ie/common/ceee_util.cc b/ceee/ie/common/ceee_util.cc
index 4d9b3d5..745e5e4 100644
--- a/ceee/ie/common/ceee_util.cc
+++ b/ceee/ie/common/ceee_util.cc
@@ -23,7 +23,8 @@ bool IsIeCeeeRegistered() {
   path += clsid;
 
   base::win::RegKey key;
-  return key.Open(HKEY_CLASSES_ROOT, path.c_str(), KEY_QUERY_VALUE);
+  return (key.Open(HKEY_CLASSES_ROOT, path.c_str(), KEY_QUERY_VALUE) ==
+      ERROR_SUCCESS);
 }
 
 }  // namespace ceee_util
diff --git a/ceee/ie/common/ie_util.cc b/ceee/ie/common/ie_util.cc
index 7c1707d..dee92d1 100644
--- a/ceee/ie/common/ie_util.cc
+++ b/ceee/ie/common/ie_util.cc
@@ -43,7 +43,7 @@ bool GetIeVersionString(std::wstring* version) {
     return false;
   base::win::RegKey key(HKEY_LOCAL_MACHINE, kIeVersionKey, KEY_READ);
   DCHECK(key.ValueExists(kIeVersionValue));
-  return key.ReadValue(kIeVersionValue, version);
+  return (key.ReadValue(kIeVersionValue, version) == ERROR_SUCCESS);
 }
 
 }  // namespace
@@ -164,9 +164,12 @@ int GetAverageAddonTimeMs(const CLSID& addon_id,
   }
 
   DWORD load_time = 0;
+  LONG result = ERROR_SUCCESS;
   if (GetIeVersion() < IEVERSION_IE9) {
-    if (!stats_key.ReadValueDW(time_prefix.c_str(), &load_time)) {
-      VLOG(1) << "Can't read time: " << time_prefix;
+    result = stats_key.ReadValueDW(time_prefix.c_str(), &load_time);
+    if (result != ERROR_SUCCESS) {
+      VLOG(1) << "Can't read time: " << time_prefix << " error: "
+              << com::LogWe(result);
       return kInvalidTime;
     }
   } else {
@@ -177,9 +180,11 @@ int GetAverageAddonTimeMs(const CLSID& addon_id,
     DWORD data_size = sizeof(values);
     DWORD data_type = REG_NONE;
 
-    if (!stats_key.ReadValue(value_name.c_str(), &values, &data_size,
-                             &data_type)) {
-      VLOG(1) << "Can't read time: " << value_name;
+    result = stats_key.ReadValue(value_name.c_str(), &values, &data_size,
+                                 &data_type);
+    if (result != ERROR_SUCCESS) {
+      VLOG(1) << "Can't read time: " << value_name << " error: "
+              << com::LogWe(result);
       return kInvalidTime;
     }
 
diff --git a/ceee/ie/common/ie_util_unittest.cc b/ceee/ie/common/ie_util_unittest.cc
index 0d0eda9..672e81c 100644
--- a/ceee/ie/common/ie_util_unittest.cc
+++ b/ceee/ie/common/ie_util_unittest.cc
@@ -70,11 +70,11 @@ TEST_F(IeUtilTest, Ie8) {
   base::win::RegKey stats_key(HKEY_CURRENT_USER, kAddonStatsRegPath, KEY_WRITE);
   ASSERT_TRUE(stats_key.Valid());
 
-  ASSERT_TRUE(stats_key.WriteValue(L"LoadTime", L"time"));
+  ASSERT_EQ(ERROR_SUCCESS, stats_key.WriteValue(L"LoadTime", L"time"));
   EXPECT_EQ(-1, GetAverageAddonLoadTimeMs(kAddonGuid));
 
   DWORD time = 11;
-  ASSERT_TRUE(stats_key.WriteValue(L"LoadTime", time));
+  ASSERT_EQ(ERROR_SUCCESS, stats_key.WriteValue(L"LoadTime", time));
 
   EXPECT_EQ(11, GetAverageAddonLoadTimeMs(kAddonGuid));
 }
@@ -89,24 +89,25 @@ TEST_F(IeUtilTest, Ie9) {
   base::win::RegKey stats_key(HKEY_CURRENT_USER, kAddonStatsRegPath, KEY_WRITE);
   ASSERT_TRUE(stats_key.Valid());
 
-  ASSERT_TRUE(stats_key.WriteValue(L"LoadTimeArray", L"time"));
+  ASSERT_EQ(ERROR_SUCCESS, stats_key.WriteValue(L"LoadTimeArray", L"time"));
   EXPECT_EQ(-1, GetAverageAddonLoadTimeMs(kAddonGuid));
 
   DWORD time[] = {1, 2, 3, 4, -1, 10};
 
-  ASSERT_TRUE(stats_key.WriteValue(L"LoadTimeArray", &time, sizeof(time[0]),
-                                   REG_BINARY));
+  ASSERT_EQ(ERROR_SUCCESS, stats_key.WriteValue(L"LoadTimeArray", &time,
+                                                sizeof(time[0]), REG_BINARY));
   EXPECT_EQ(-1, GetAverageAddonLoadTimeMs(kAddonGuid));
 
-  ASSERT_TRUE(stats_key.WriteValue(L"LoadTimeArray", &time, 10, REG_BINARY));
+  ASSERT_EQ(ERROR_SUCCESS,
+      stats_key.WriteValue(L"LoadTimeArray", &time, 10, REG_BINARY));
   EXPECT_EQ(-1, GetAverageAddonLoadTimeMs(kAddonGuid));
 
-  ASSERT_TRUE(stats_key.WriteValue(L"LoadTimeArray", &time, 4 * sizeof(time[0]),
-                                   REG_BINARY));
+  ASSERT_EQ(ERROR_SUCCESS, stats_key.WriteValue(L"LoadTimeArray",
+      &time, 4 * sizeof(time[0]), REG_BINARY));
   EXPECT_EQ(2, GetAverageAddonLoadTimeMs(kAddonGuid));
 
-  ASSERT_TRUE(stats_key.WriteValue(L"LoadTimeArray", time, sizeof(time),
-                                   REG_BINARY));
+  ASSERT_EQ(ERROR_SUCCESS, stats_key.WriteValue(L"LoadTimeArray", time,
+                                                sizeof(time), REG_BINARY));
   EXPECT_EQ(4, GetAverageAddonLoadTimeMs(kAddonGuid));
 }
 
diff --git a/ceee/ie/plugin/toolband/toolband_proxy.cc b/ceee/ie/plugin/toolband/toolband_proxy.cc
index a228cda..d6094cb 100644
--- a/ceee/ie/plugin/toolband/toolband_proxy.cc
+++ b/ceee/ie/plugin/toolband/toolband_proxy.cc
@@ -51,11 +51,12 @@ void CheckAsyncIidRegistered(const IID& iid, const IID& async_iid) {
       L"Interface\\%ls\\AsynchronousInterface", iid_str.c_str());
 
   base::win::RegKey key;
-  if (key.Open(HKEY_CLASSES_ROOT, key_name.c_str(), KEY_READ)) {
+  if (key.Open(HKEY_CLASSES_ROOT, key_name.c_str(), KEY_READ) ==
+      ERROR_SUCCESS) {
     // It's registered, the rest of this block is debug checking that
     // the correct IID is indeed registered for the async interface.
     std::wstring async_iid_str;
-    DCHECK(key.ReadValue(NULL, &async_iid_str));
+    DCHECK_EQ(ERROR_SUCCESS, key.ReadValue(NULL, &async_iid_str));
     IID read_async_iid;
     DCHECK(SUCCEEDED(::IIDFromString(async_iid_str.c_str(), &read_async_iid)));
     DCHECK(read_async_iid == async_iid);
diff --git a/ceee/installer_dll/installer_helper.cc b/ceee/installer_dll/installer_helper.cc
index 2128228..5c65a62 100644
--- a/ceee/installer_dll/installer_helper.cc
+++ b/ceee/installer_dll/installer_helper.cc
@@ -227,10 +227,11 @@ HRESULT RegisterFirefoxCeee(bool do_register) {
 
     if (file_util::PathExists(path)) {
       base::win::RegKey key(HKEY_LOCAL_MACHINE, L"SOFTWARE", KEY_WRITE);
-      if (key.CreateKey(L"Mozilla", KEY_WRITE) &&
-          key.CreateKey(L"Firefox", KEY_WRITE) &&
-          key.CreateKey(L"extensions", KEY_WRITE) &&
-          key.WriteValue(kCeeeFirefoxExtensionName, path.value().c_str())) {
+      if ((key.CreateKey(L"Mozilla", KEY_WRITE) == ERROR_SUCCESS) &&
+          (key.CreateKey(L"Firefox", KEY_WRITE) == ERROR_SUCCESS) &&
+          (key.CreateKey(L"extensions", KEY_WRITE) == ERROR_SUCCESS) &&
+          (key.WriteValue(kCeeeFirefoxExtensionName, path.value().c_str()))
+              == ERROR_SUCCESS) {
         hr = S_OK;
       } else {
         hr = com::AlwaysErrorFromLastError();
@@ -239,13 +240,12 @@ HRESULT RegisterFirefoxCeee(bool do_register) {
   } else {
     hr = S_OK;  // OK if not found, then there's nothing to do
     base::win::RegKey key(HKEY_LOCAL_MACHINE, L"SOFTWARE", KEY_READ);
-    if (key.OpenKey(L"Mozilla", KEY_READ) &&
-        key.OpenKey(L"Firefox", KEY_READ) &&
-        key.OpenKey(L"extensions", KEY_WRITE) &&
+    if ((key.OpenKey(L"Mozilla", KEY_READ) == ERROR_SUCCESS) &&
+        (key.OpenKey(L"Firefox", KEY_READ) == ERROR_SUCCESS) &&
+        (key.OpenKey(L"extensions", KEY_WRITE) == ERROR_SUCCESS) &&
         key.ValueExists(kCeeeFirefoxExtensionName)) {
-      if (!key.DeleteValue(kCeeeFirefoxExtensionName)) {
-        hr = com::AlwaysErrorFromLastError();
-      }
+      LONG result = key.DeleteValue(kCeeeFirefoxExtensionName);
+      hr = HRESULT_FROM_WIN32(result);
     }
   }
 
@@ -258,7 +258,8 @@ HRESULT SetCeeeChannelModifier(bool new_value) {
   std::wstring reg_key(ceee_module_util::GetCromeFrameClientStateKey());
 
   base::win::RegKey key;
-  if (!key.Open(HKEY_LOCAL_MACHINE, reg_key.c_str(), KEY_ALL_ACCESS)) {
+  if (key.Open(HKEY_LOCAL_MACHINE, reg_key.c_str(), KEY_ALL_ACCESS) !=
+      ERROR_SUCCESS) {
     // Omaha didn't install the key.  Perhaps no Omaha?  That's ok.
     return S_OK;
   }
diff --git a/ceee/testing/utils/test_utils.cc b/ceee/testing/utils/test_utils.cc
index 282416a..59ad886 100644
--- a/ceee/testing/utils/test_utils.cc
+++ b/ceee/testing/utils/test_utils.cc
@@ -192,8 +192,10 @@ void ScopedRegistryOverride::Override() {
   ASSERT_TRUE(err == ERROR_SUCCESS || err == ERROR_FILE_NOT_FOUND);
 
   // Create the keys we're redirecting HKCU and HKLM to.
-  ASSERT_TRUE(hkcu_.Create(HKEY_CURRENT_USER, kHKCUReplacement, KEY_READ));
-  ASSERT_TRUE(hklm_.Create(HKEY_CURRENT_USER, kHKLMReplacement, KEY_READ));
+  ASSERT_EQ(ERROR_SUCCESS,
+      hkcu_.Create(HKEY_CURRENT_USER, kHKCUReplacement, KEY_READ));
+  ASSERT_EQ(ERROR_SUCCESS,
+      hklm_.Create(HKEY_CURRENT_USER, kHKLMReplacement, KEY_READ));
 
   // Switch keys.
   ASSERT_EQ(ERROR_SUCCESS, ::RegOverridePredefKey(HKEY_CURRENT_USER,