Regkey functions return error code instead of bool

chrome/browser/autofill/autofill_ie_toolbar_import_win.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/autofill/autofill_ie_toolbar_import_win.h"
 
 #include "base/basictypes.h"
 #include "base/string16.h"
 #include "base/win/registry.h"
 #include "chrome/browser/autofill/autofill_profile.h"
@@ skipping 39 matching lines @@
   for (size_t i = 0; i < salt.length(); ++i) {
     if (salt[i] != i + 1)
       return false;
   }
   return true;
 }
 
 string16 ReadAndDecryptValue(RegKey* key, const wchar_t* value_name) {
   DWORD data_type = REG_BINARY;
   DWORD data_size = 0;
-  if (!key->ReadValue(value_name, NULL, &data_size, &data_type) ||
-      !data_size || data_type != REG_BINARY)
+  LONG result = key->ReadValue(value_name, NULL, &data_size, &data_type);

[brettw, 2011/01/10 20:42:28]

In other places you didn't introduce the extra var and just did the ReadValue
inside the if statement. I think that's a bit more readable since it's clear
that the return value isn't used any other place.

[amit, 2011/01/12 04:11:23]

My logic was to directly check for ERROR_SUCCESS if it fits on the same line. If
not then it's more readable to declare a temporary. 

For. e.g. in this particular change itself, wrapping ERROR_SUCCESS on the next
line would make it hard to spot the data_size and data_type checks. Whereas as
they stand now, the conditions are obvious in a single glance.

I am not attached to this logic, however, and willing to be convinced otherwise
:)

[grt (UTC plus 2), 2011/01/12 15:06:38]

FWIW, I think I spent more time trying to figure out why a local was used in
some places and not in others than I would have spent reading wrapped lines.

+  if ((result != ERROR_SUCCESS) || !data_size || data_type != REG_BINARY)
     return string16();
   std::vector<uint8> data;
   data.resize(data_size);
-  if (key->ReadValue(value_name, &(data[0]), &data_size, &data_type)) {
+  result = key->ReadValue(value_name, &(data[0]), &data_size, &data_type);
+  if (result == ERROR_SUCCESS) {
     std::string out_data;
     if (DecryptData(data, &out_data)) {
       // The actual data is in UTF16 already.
       if (!(out_data.size() & 1) && (out_data.size() > 2) &&
           !out_data[out_data.size() - 1] && !out_data[out_data.size() - 2]) {
         return string16(
             reinterpret_cast<const wchar_t *>(out_data.c_str()));
       }
     }
   }
@@ skipping 42 matching lines @@
                          RegKey* key,
                          const RegToFieldMap& reg_to_field ) {
   DCHECK(profile != NULL);
   if (!key->Valid())
     return false;
 
   bool has_non_empty_fields = false;
 
   for (uint32 value_index = 0; value_index < key->ValueCount(); ++value_index) {
     std::wstring value_name;
-    if (!key->ReadName(value_index, &value_name))
+    if (key->ReadName(value_index, &value_name) != ERROR_SUCCESS)
       continue;
     RegToFieldMap::const_iterator it = reg_to_field.find(value_name);
     if (it == reg_to_field.end())
       continue;  // This field is not imported.
     string16 field_value = ReadAndDecryptValue(key, value_name.c_str());
     if (!field_value.empty()) {
       has_non_empty_fields = true;
       if (it->second == CREDIT_CARD_NUMBER) {
         field_value = DecryptCCNumber(field_value);
       }
@@ skipping 108 matching lines @@
   }
   return (profiles->size() + credit_cards->size()) > 0;
 }
 
 bool ImportAutofillDataWin(PersonalDataManager* pdm) {
   AutoFillImporter *importer = new AutoFillImporter(pdm);
   // importer will self delete.
   return importer->ImportProfiles();
 }