Regkey functions return error code instead of bool

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




Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71768

[brettw, 2011-01-10 20:42:27]

Which places are the ones you changed that did workarounds for the lack of
return values?

http://codereview.chromium.org/6090006/diff/141066/base/win/registry.h
File base/win/registry.h (right):

http://codereview.chromium.org/6090006/diff/141066/base/win/registry.h#newcode14
base/win/registry.h:14: // Please ignore this part. This temporary hack exists
What's the plan for removing this? Should it be a TODO?

http://codereview.chromium.org/6090006/diff/141066/base/win/registry.h#newcode23
base/win/registry.h:23: private:
Indent one more space.

http://codereview.chromium.org/6090006/diff/141066/base/win/registry.h#newcode91
base/win/registry.h:91: GONG ReadValueQW(const wchar_t* name, int64* value)
const;
What do you think about calling this ReadValue64 or even ReadInt64? We've never
used quadword in our codebase so this looks a bit out of place, whereas "64" is
much more common.

http://codereview.chromium.org/6090006/diff/141066/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_ie_toolbar_import_win.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_ie_toolbar_import_win.cc:60: LONG result =
key->ReadValue(value_name, NULL, &data_size, &data_type);
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.

[Sigurður Ásgeirsson, 2011-01-10 21:02:42]

Thanks for doing this.

http://codereview.chromium.org/6090006/diff/141066/base/win/registry.h
File base/win/registry.h (right):

http://codereview.chromium.org/6090006/diff/141066/base/win/registry.h#newcode90
base/win/registry.h:90: GONG ReadValueDW(const wchar_t* name, DWORD* value)
const;
The way you use this in ceee relies upon the fact that *value is untouched
except on success. Since this isn't documented or tested, it falls into
implementation detail.
Either change the usage or (better) document and test this behavior?

http://codereview.chromium.org/6090006/diff/141066/ceee/ie/common/ceee_module...
File ceee/ie/common/ceee_module_util.cc (right):

http://codereview.chromium.org/6090006/diff/141066/ceee/ie/common/ceee_module...
ceee/ie/common/ceee_module_util.cc:47: hkcu.ReadValueDW(key, &value);
This relies upon undocumented, untested behavior of RegKey, namely that the
value is untouched on errors. Either fix the usage or else document and test the
behavior?
A regression here would not be fun to track down :).

http://codereview.chromium.org/6090006/diff/141066/ceee/ie/common/ceee_module...
ceee/ie/common/ceee_module_util.cc:62: << " error: " << write_result;
We use 

com::LogWe(<windows error code>)

to log human readable windows errors.

http://codereview.chromium.org/6090006/diff/141066/ceee/ie/common/ceee_module...
ceee/ie/common/ceee_module_util.cc:338: << kValueName << " error: " << result;
com::LogWe

http://codereview.chromium.org/6090006/diff/141066/ceee/ie/common/ie_util.cc
File ceee/ie/common/ie_util.cc (right):

http://codereview.chromium.org/6090006/diff/141066/ceee/ie/common/ie_util.cc#...
ceee/ie/common/ie_util.cc:168: LONG result =
stats_key.ReadValueDW(time_prefix.c_str(), &load_time);
ugh, I guess we rely on this everywhere. Can I still ask you to document and
test this?

http://codereview.chromium.org/6090006/diff/141066/ceee/ie/common/ie_util.cc#...
ceee/ie/common/ie_util.cc:184: VLOG(1) << "Can't read time: " << value_name;
com::LogWe(result) now that we have the error in question, please?

http://codereview.chromium.org/6090006/diff/141066/ceee/installer_dll/install...
File ceee/installer_dll/installer_helper.cc (right):

http://codereview.chromium.org/6090006/diff/141066/ceee/installer_dll/install...
ceee/installer_dll/installer_helper.cc:229: if ((key.CreateKey(L"Mozilla",
KEY_WRITE) == ERROR_SUCCESS) &&
looks like this would be simpler with a path to a single CreateKey invocation?

[robertshield, 2011-01-10 21:34:36]

http://codereview.chromium.org/6090006/diff/141066/base/win/registry.h
File base/win/registry.h (right):

http://codereview.chromium.org/6090006/diff/141066/base/win/registry.h#newcode24
base/win/registry.h:24: operator bool () {return false;}
If you intend this to be checked in, consider adding a big visible comment
saying something like:
"If you hit a compile error here, you attempted to use the return value as a
bool. Don't do that."

[brettw, 2011-01-10 23:17:00]

I looked at about half of this patch and so far, every callsite is now less
readable and often more lines of code. I was originally OK if you wanted to pass
up error information, but I'd like to understand how much benefit this actually
has before forcing all callers to check for ERROR_SUCCESS.

[amit, 2011-01-10 23:32:05]

In general, majority of callers should be aware of (and handle) registry API
failures for different reasons, say: key does not exist or the caller is
asking for KEY_ALL_ACCESS but allowed to open only as read only.  Currently,
the helpers simply return a bool so most of the usage today is 'compact' at
the cost of analyzing things further.  Having return code enables you to
think about handling different failure cases so my guess is that the new
usage will follow a different pattern.

On Mon, Jan 10, 2011 at 3:17 PM, <brettw@chromium.org> wrote:

> I looked at about half of this patch and so far, every callsite is now less
> readable and often more lines of code. I was originally OK if you wanted to
> pass
> up error information, but I'd like to understand how much benefit this
> actually
> has before forcing all callers to check for ERROR_SUCCESS.
>
>
> http://codereview.chromium.org/6090006/
>

[brettw, 2011-01-10 23:47:52]

I just clicked on two random files that seem representative:
delete_after_reboot_helper.cc and firefox_importer_utils_win.cc. What error
handling should be added to these functions?

I usually tell people not to do this type of error handling. A typical caller
has absolutely no idea what errors they might need to check for or what they
should do in those cases other than returning false. Just returning a bool
answers both of these questions and makes the code prettier (the fact that the
helper class exists to prevent common mistakes for bool indicates to me that the
newer API is more confusing).

[amit, 2011-01-11 00:14:48]

delete_after_reboot_helper.cc:
  - Please refer to GetPendingMovesValue where we are working around since
API return values are not available.
  - RemoveFromMovesPendingReboot: note the (anti-)pattern: base::win::RegKey
session_manager_key(...) followed by if (!session_manager_key.Handle())
{....}  Calling RegOpenKey in ctor is not ideal and knowing why it failed
would be certainly helpful in the LOG(ERROR).

firefox_importer_utils_win.cc:
  - GetFirefoxInstallPathFromRegistry: This is a best effort to read the
path. No need to check any return value here. Just call ReadValue and check
if buffer is valid.

As a general principle, although we are using convenience wrappers around
APIs, the callers should be cognizant about why it could fail.

On Mon, Jan 10, 2011 at 3:47 PM, <brettw@chromium.org> wrote:

> I just clicked on two random files that seem representative:
> delete_after_reboot_helper.cc and firefox_importer_utils_win.cc. What error
> handling should be added to these functions?
>
> I usually tell people not to do this type of error handling. A typical
> caller
> has absolutely no idea what errors they might need to check for or what
> they
> should do in those cases other than returning false. Just returning a bool
> answers both of these questions and makes the code prettier (the fact that
> the
> helper class exists to prevent common mistakes for bool indicates to me
> that the
> newer API is more confusing).
>
>
> http://codereview.chromium.org/6090006/
>

[grt (UTC plus 2), 2011-01-11 03:51:30]

Thanks for the work here, Amit.  This will allow us to make further cleanups to
the various registry-related WorkItem classes.  Yay!

http://codereview.chromium.org/6090006/diff/141066/chrome/browser/background_...
File chrome/browser/background_mode_manager_win.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/browser/background_...
chrome/browser/background_mode_manager_win.cc:43: if (result != ERROR_SUCCESS)
DCHECK_EQ(ERROR_SUCCESS, result) << "Failed..." << com::LogWe(result);

http://codereview.chromium.org/6090006/diff/141066/chrome/browser/background_...
chrome/browser/background_mode_manager_win.cc:68: if (result != ERROR_SUCCESS)
DCHECK_EQ as above

http://codereview.chromium.org/6090006/diff/141066/chrome/browser/importer/ie...
File chrome/browser/importer/ie_importer.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/browser/importer/ie...
chrome/browser/importer/ie_importer.cc:271: std::vector<unsigned char> value;
Do you mind doing a little extra cleanup here?  This is doing a bunch of
unneeded allocations, deallocations, and memcpys.  If you don't mind, please:
- move the "password_info" local up out of the while loop.
- remove the "value" local
- replace value.resize with password_info.encrypted_data.resize
- replace value.front with password_info.encrypted_data.front

http://codereview.chromium.org/6090006/diff/141066/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_provider_delegate_win.cc
(right):

http://codereview.chromium.org/6090006/diff/141066/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_provider_delegate_win.cc:129: *result
= local_result != 0;
Only modify *result when ret != false.

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/dele...
File chrome/installer/util/delete_reg_value_work_item.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/dele...
chrome/installer/util/delete_reg_value_work_item.cc:46: if
(!key.ValueExists(value_name_.c_str())) {
It's less work and less code to remove this call to ValueExists and hande
ERROR_FILE_NOT_FOUND from the later calls to ReadValue*, no?

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
File chrome/installer/util/google_update_settings.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:103: RegKey
hklm_key(HKEY_LOCAL_MACHINE, reg_path.c_str(), KEY_READ);
how about use key.Open() rather than making a new instance?

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:104:
hklm_key.ReadValueDW(google_update::kRegUsageStatsField, &value);
I prefer checking the return value here rather than relying on "value" remaining
0.

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:118: RegKey
key_hkcu(HKEY_CURRENT_USER, reg_path.c_str(), KEY_READ | KEY_WRITE);
Change to use only one RegKey instance?

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:265: if (result !=
ERROR_SUCCESS)
result == ERROR_SUCCESS

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:266:
key.CreateKey(product_guid.c_str(), KEY_ALL_ACCESS);
result = key.CreateKey(...)

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/inst...
File chrome/installer/util/install_util.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/inst...
chrome/installer/util/install_util.cc:92: key.Close();
Remove

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/prod...
File chrome/installer/util/product.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/prod...
chrome/installer/util/product.cc:133: result =
key.ReadValueDW(google_update::kRegMSIField, &msi_value);
I prefer an if (result == ERROR_SUCCESS) before relying on msi_value.

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/prod...
chrome/installer/util/product.cc:156: LOG(ERROR) << "Failed to Open or Write MSI
value to client state key."
LOG_IF(ERROR, result != ERROR_SUCCESS) << ...

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/prod...
chrome/installer/util/product.cc:157: << " error: " << result;
com::LogWe(result)  (i love this!)

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/set_...
File chrome/installer/util/set_reg_value_work_item.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/set_...
chrome/installer/util/set_reg_value_work_item.cc:56: LOG(ERROR) << "can not open
" << key_path_ << " error: " << result;
com::LogWe here and everywhere

http://codereview.chromium.org/6090006/diff/141066/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome_frame/chrome_tab.cc...
chrome_frame/chrome_tab.cc:444: DLOG(ERROR) << __FUNCTION__ << ": " <<
kPostPlatformUAKey;
com::LogWe(result) also

http://codereview.chromium.org/6090006/diff/141066/chrome_frame/chrome_tab.cc...
chrome_frame/chrome_tab.cc:741: if (result == ERROR_SUCCESS)
is this change in behavior intentional?

http://codereview.chromium.org/6090006/diff/141066/chrome_frame/chrome_tab.cc...
chrome_frame/chrome_tab.cc:746: result =
key.DeleteValue(L"ChromeTab.ChromeActiveDocument.1");
Don't we want to do the second delete even if the first one fails?  Uninstall is
usually best-effort rather than fail-fast.

[brettw, 2011-01-11 05:59:45]

Have you considered storing the return value for the callers that are actually
interested? This will keep the many callers who don't care clean, and the
callers who do care might be cleaner too. In the case of
delete_after_reboot_helper.cc:

RegKey key;
LONG err = key.Open(predefined_root_, key_path_.c_str(), KEY_READ);
if (err == ERROR_FILE_NOT_FOUND) {

vs.

base::win::RegKey key(predefined_root_, key_path_.c_str(), KEY_READ);
if (key.GetLastError() == ERROR_FILE_NOT_FOUND) {


Or in ceee_module_util.cc:

LONG write_result = hkcu.WriteValue(key, &dword_value_representation,
                                    sizeof(dword_value_representation),
                                    REG_DWORD);
LOG_IF(ERROR, write_result != ERROR_SUCCESS)
    << "Failed to write a registry key: " << key
    << " error: " << write_result;

vs.

if (!hkcu.WriteValue(key, &dword_value_representation,
                     sizeof(dword_value_representation),
                     REG_DWORD)) {
  LOG(ERROR) << "Failed to write a registry key: " << key
             << " error: " << hkcu.GetLastError();
}

In both cases the second looks cleaner and more like "Chrome code" to me.

[grt (UTC plus 2), 2011-01-11 14:29:27]

On Tue, Jan 11, 2011 at 12:59 AM, <brettw@chromium.org> wrote:

> Have you considered storing the return value for the callers that are
> actually
> interested? This will keep the many callers who don't care clean, and the
> callers who do care might be cleaner too.


How do you feel about the associated increase in RegKey instance size (4
bytes)?  For the most part, they're on the stack and there don't tend to be
too many of them, so my gut tells me it probably wouldn't be an issue.

[brettw, 2011-01-11 16:51:04]

On Tue, Jan 11, 2011 at 6:29 AM, Greg Thompson <grt@chromium.org> wrote:
> On Tue, Jan 11, 2011 at 12:59 AM, <brettw@chromium.org> wrote:
>>
>> Have you considered storing the return value for the callers that are
>> actually
>> interested? This will keep the many callers who don't care clean, and the
>> callers who do care might be cleaner too.
>
> How do you feel about the associated increase in RegKey instance size (4
> bytes)?  For the most part, they're on the stack and there don't tend to be
> too many of them, so my gut tells me it probably wouldn't be an issue.

Yeah, I wouldn't care if the size was half a K larger.

Brett

[amit, 2011-01-11 18:45:47]

I did consider it and it was tempting since it's a much simpler change.
 Then I thought about what would be the trade off if I am starting out with
a new RegKey class:

Advantages:

   - Terse error checking and compact expressions.

Vs disadvantages:

   - All the present and future helpers in RegKey class will have to
   correctly preserve the last error.  It's easy to miss this and it can lead
   to subtle errors. For e.g. DWORD RegKey::ValueCount() it's easy for someone
   writing this to incorrectly set or reset last error.
   - Unnecessarily brittle API. Registry functions are thread safe but with
   RegKey::GetLastError this class won't be.

I too feel that, if (reg_key.Open(...) == ERROR_SUCCESS) is ugly. I guess
Microsoft does feel the same way too and that's why they invented SUCCEEDED
or FAILED for HRESULT.  Resulting if (SUCCEEDED(CoCreateInstance(...))  is
less ugly, still not saying it's pretty :)  However, it's a much better API
than if (!CreateWindow(...)) { ... GetLastError(); }

Ultimately, even if it is more work, it seemed to me that the a robust API
is better.

On Mon, Jan 10, 2011 at 9:59 PM, <brettw@chromium.org> wrote:

> Have you considered storing the return value for the callers that are
> actually
> interested? This will keep the many callers who don't care clean, and the
> callers who do care might be cleaner too. In the case of
>
> delete_after_reboot_helper.cc:
>
> RegKey key;
> LONG err = key.Open(predefined_root_, key_path_.c_str(), KEY_READ);
> if (err == ERROR_FILE_NOT_FOUND) {
>
> vs.
>
> base::win::RegKey key(predefined_root_, key_path_.c_str(), KEY_READ);
> if (key.GetLastError() == ERROR_FILE_NOT_FOUND) {
>
>
> Or in ceee_module_util.cc:
>
> LONG write_result = hkcu.WriteValue(key, &dword_value_representation,
>                                    sizeof(dword_value_representation),
>                                    REG_DWORD);
> LOG_IF(ERROR, write_result != ERROR_SUCCESS)
>    << "Failed to write a registry key: " << key
>    << " error: " << write_result;
>
> vs.
>
> if (!hkcu.WriteValue(key, &dword_value_representation,
>                     sizeof(dword_value_representation),
>                     REG_DWORD)) {
>  LOG(ERROR) << "Failed to write a registry key: " << key
>             << " error: " << hkcu.GetLastError();
> }
>
> In both cases the second looks cleaner and more like "Chrome code" to me.
>
>
> http://codereview.chromium.org/6090006/
>

[brettw, 2011-01-11 23:39:02]

On Tue, Jan 11, 2011 at 10:45 AM, Amit Joshi <amit@chromium.org> wrote:
> I did consider it and it was tempting since it's a much simpler change.
>  Then I thought about what would be the trade off if I am starting out with
> a new RegKey class:
> Advantages:
>
> Terse error checking and compact expressions.
>
> Vs disadvantages:
>
> All the present and future helpers in RegKey class will have to correctly
> preserve the last error.  It's easy to miss this and it can lead to subtle
> errors. For e.g. DWORD RegKey::ValueCount() it's easy for someone writing
> this to incorrectly set or reset last error.

I guess we'll have to be careful when writing RegKey methods, but it
seems like we almost never add more, and the new ones are copied &
pasted from the old ones. So I'm not so worried about this.

> Unnecessarily brittle API. Registry functions are thread safe but with
> RegKey::GetLastError this class won't be.

None of our classes are threadsafe unless it has a huge THIS CLASS IS
THREADSAFE at the top. I'm a little surprised that the current one is
threadsafe, and I don't think being not threadsafe makes it "brittle."

I'm still not clear on whether you plan to remove the GONG thing soon
or it's intended to be permanent. If it's supposed to be permanent, I
think we need to do some cleanup and not use #defines. This is
something that you didn't cover in the list: the new API is actually
much more fragile and the obvious usage is wrong, were it not for this
compiler magic to make the obvious thing an error.

Brett

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

new patch up, PTAL.

http://codereview.chromium.org/6090006/diff/141066/base/win/registry.h
File base/win/registry.h (right):

http://codereview.chromium.org/6090006/diff/141066/base/win/registry.h#newcode14
base/win/registry.h:14: // Please ignore this part. This temporary hack exists
On 2011/01/10 20:42:28, brettw wrote:
> What's the plan for removing this? Should it be a TODO?
Todo added. 

This was to help catch all the usages and help catch new usages while merging. I
was planning to remove this before checkin. 

If you wish, I could leave this on for a day or two to protect others that have
changes in flight.

http://codereview.chromium.org/6090006/diff/141066/base/win/registry.h#newcode23
base/win/registry.h:23: private:
On 2011/01/10 20:42:28, brettw wrote:
> Indent one more space.

Done.

http://codereview.chromium.org/6090006/diff/141066/base/win/registry.h#newcode24
base/win/registry.h:24: operator bool () {return false;}
On 2011/01/10 21:34:37, robertshield wrote:
> If you intend this to be checked in, consider adding a big visible comment
> saying something like:
> "If you hit a compile error here, you attempted to use the return value as a
> bool. Don't do that."

Done.

http://codereview.chromium.org/6090006/diff/141066/base/win/registry.h#newcode90
base/win/registry.h:90: GONG ReadValueDW(const wchar_t* name, DWORD* value)
const;
On 2011/01/10 21:02:43, Ruðrugis wrote:
> The way you use this in ceee relies upon the fact that *value is untouched
> except on success. Since this isn't documented or tested, it falls into
> implementation detail.
> Either change the usage or (better) document and test this behavior? 

I think it's not proper for an API to return failure but cause side effects in
the return arg. So if it's OK with you, I would prefer to document, test and
enforce this behavior.

http://codereview.chromium.org/6090006/diff/141066/base/win/registry.h#newcode91
base/win/registry.h:91: GONG ReadValueQW(const wchar_t* name, int64* value)
const;
On 2011/01/10 20:42:28, brettw wrote:
> What do you think about calling this ReadValue64 or even ReadInt64? We've
never
> used quadword in our codebase so this looks a bit out of place, whereas "64"
is
> much more common.
Yeah ReadInt64 is much more readable. Changed.

http://codereview.chromium.org/6090006/diff/141066/ceee/ie/common/ceee_module...
File ceee/ie/common/ceee_module_util.cc (right):

http://codereview.chromium.org/6090006/diff/141066/ceee/ie/common/ceee_module...
ceee/ie/common/ceee_module_util.cc:62: << " error: " << write_result;
On 2011/01/10 21:02:43, Ruðrugis wrote:
> We use 
> 
> com::LogWe(<windows error code>)
> 
> to log human readable windows errors.

Done.

http://codereview.chromium.org/6090006/diff/141066/ceee/ie/common/ie_util.cc
File ceee/ie/common/ie_util.cc (right):

http://codereview.chromium.org/6090006/diff/141066/ceee/ie/common/ie_util.cc#...
ceee/ie/common/ie_util.cc:184: VLOG(1) << "Can't read time: " << value_name;
On 2011/01/10 21:02:43, Ruðrugis wrote:
> com::LogWe(result) now that we have the error in question, please?

Done.

http://codereview.chromium.org/6090006/diff/141066/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_ie_toolbar_import_win.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_ie_toolbar_import_win.cc:60: LONG result =
key->ReadValue(value_name, NULL, &data_size, &data_type);
On 2011/01/10 20:42:28, brettw wrote:
> 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.

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
:)

http://codereview.chromium.org/6090006/diff/141066/chrome/browser/background_...
File chrome/browser/background_mode_manager_win.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/browser/background_...
chrome/browser/background_mode_manager_win.cc:43: if (result != ERROR_SUCCESS)
On 2011/01/11 03:51:30, grt wrote:
> DCHECK_EQ(ERROR_SUCCESS, result) << "Failed..." << com::LogWe(result);

Done.

http://codereview.chromium.org/6090006/diff/141066/chrome/browser/background_...
chrome/browser/background_mode_manager_win.cc:68: if (result != ERROR_SUCCESS)
On 2011/01/11 03:51:30, grt wrote:
> DCHECK_EQ as above

Done.

http://codereview.chromium.org/6090006/diff/141066/chrome/browser/importer/ie...
File chrome/browser/importer/ie_importer.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/browser/importer/ie...
chrome/browser/importer/ie_importer.cc:271: std::vector<unsigned char> value;
On 2011/01/11 03:51:30, grt wrote:
> Do you mind doing a little extra cleanup here?  This is doing a bunch of
> unneeded allocations, deallocations, and memcpys.  If you don't mind, please:
> - move the "password_info" local up out of the while loop.
> - remove the "value" local
> - replace value.resize with password_info.encrypted_data.resize
> - replace value.front with password_info.encrypted_data.front

Done.

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
File chrome/installer/util/google_update_settings.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:103: RegKey
hklm_key(HKEY_LOCAL_MACHINE, reg_path.c_str(), KEY_READ);
On 2011/01/11 03:51:30, grt wrote:
> how about use key.Open() rather than making a new instance?

Done.

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:118: RegKey
key_hkcu(HKEY_CURRENT_USER, reg_path.c_str(), KEY_READ | KEY_WRITE);
On 2011/01/11 03:51:30, grt wrote:
> Change to use only one RegKey instance?

Done.

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:265: if (result !=
ERROR_SUCCESS)
On 2011/01/11 03:51:30, grt wrote:
> result == ERROR_SUCCESS
Are you sure? As per the old code, key.CreateKey will execute only if key.Open
fails.

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/inst...
File chrome/installer/util/install_util.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/inst...
chrome/installer/util/install_util.cc:92: key.Close();
On 2011/01/11 03:51:30, grt wrote:
> Remove

Done.

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/prod...
File chrome/installer/util/product.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/prod...
chrome/installer/util/product.cc:156: LOG(ERROR) << "Failed to Open or Write MSI
value to client state key."
On 2011/01/11 03:51:30, grt wrote:
> LOG_IF(ERROR, result != ERROR_SUCCESS) << ...

Done.

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/prod...
chrome/installer/util/product.cc:157: << " error: " << result;
On 2011/01/11 03:51:30, grt wrote:
> com::LogWe(result)  (i love this!)
com::LogWe is cool and I have changed to it in CEEE. However, I am not sure if
we want to make installer depend on a CEEE lib. Maybe a first step is to factor
that out in a common lib elsewhere?

http://codereview.chromium.org/6090006/diff/141066/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome_frame/chrome_tab.cc...
chrome_frame/chrome_tab.cc:741: if (result == ERROR_SUCCESS)
On 2011/01/11 03:51:30, grt wrote:
> is this change in behavior intentional?
It's not. good catch!

http://codereview.chromium.org/6090006/diff/141066/chrome_frame/chrome_tab.cc...
chrome_frame/chrome_tab.cc:746: result =
key.DeleteValue(L"ChromeTab.ChromeActiveDocument.1");
On 2011/01/11 03:51:30, grt wrote:
> Don't we want to do the second delete even if the first one fails?  Uninstall
is
> usually best-effort rather than fail-fast.

Done.

[Sigurður Ásgeirsson, 2011-01-12 14:04:19]

lgtm for CEEE portion. Thanks again for doing this.

http://codereview.chromium.org/6090006/diff/141066/base/win/registry.h
File base/win/registry.h (right):

http://codereview.chromium.org/6090006/diff/141066/base/win/registry.h#newcode14
base/win/registry.h:14: // Please ignore this part. This temporary hack exists
On 2011/01/12 04:11:23, amit wrote:
> On 2011/01/10 20:42:28, brettw wrote:
> > What's the plan for removing this? Should it be a TODO?
> Todo added. 
> 
> This was to help catch all the usages and help catch new usages while merging.
I
> was planning to remove this before checkin. 
> 
> If you wish, I could leave this on for a day or two to protect others that
have
> changes in flight.

Maybe not a bad idea to leave this in place for a bit, but perhaps only in debug
mode?

http://codereview.chromium.org/6090006/diff/141066/base/win/registry.h#newcode90
base/win/registry.h:90: GONG ReadValueDW(const wchar_t* name, DWORD* value)
const;
On 2011/01/12 04:11:23, amit wrote:
> On 2011/01/10 21:02:43, Ruðrugis wrote:
> > The way you use this in ceee relies upon the fact that *value is untouched
> > except on success. Since this isn't documented or tested, it falls into
> > implementation detail.
> > Either change the usage or (better) document and test this behavior? 
> 
> I think it's not proper for an API to return failure but cause side effects in
> the return arg. So if it's OK with you, I would prefer to document, test and
> enforce this behavior.

I couldn't agree more - SGTM!

http://codereview.chromium.org/6090006/diff/30005/base/win/registry.h#newcode28
base/win/registry.h:28: return g.value() == l;
nit: ws around {}

[robertshield, 2011-01-12 15:01:29]

Mostly style nits, one thing that looks like a regression (|| instead of &&).

In general, I find adding temporary vars and then sometimes breaking up
conditions into multiple if statements a bit harder to read (I have to think
about what they're doing slightly longer). I didn't come across many egregious
cases here.

http://codereview.chromium.org/6090006/diff/30005/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/6090006/diff/30005/chrome_frame/chrome_tab.cc#...
chrome_frame/chrome_tab.cc:323: LONG ret = run_once.Create(hive, kRunOnce,
KEY_READ | KEY_WRITE);
nit: 
if (run_once.Create(hive, kRunOnce, KEY_READ | KEY_WRITE) == ERROR_SUCCESS) {
would fit on one line, allowing you to do away with ret, at least at this scope
level.

http://codereview.chromium.org/6090006/diff/30005/chrome_frame/chrome_tab.cc#...
chrome_frame/chrome_tab.cc:748: return (result2 == ERROR_SUCCESS) || (result2 ==
ERROR_SUCCESS);
The || should be an &&: previously both writes/deletes had to succeed for this
to return true, now only one of them has to succeed.

http://codereview.chromium.org/6090006/diff/30005/chrome_frame/crash_reportin...
File chrome_frame/crash_reporting/crash_metrics.cc (right):

http://codereview.chromium.org/6090006/diff/30005/chrome_frame/crash_reportin...
chrome_frame/crash_reporting/crash_metrics.cc:34: KEY_SET_VALUE);
nit: the temporary here makes this harder to read imho, consider formatting it
similarly to GetMetric() a few lines down.

http://codereview.chromium.org/6090006/diff/30005/chrome_frame/ready_mode/int...
File chrome_frame/ready_mode/internal/registry_ready_mode_state.cc (right):

http://codereview.chromium.org/6090006/diff/30005/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:111: }
More nitty whining:
I found the original code easier to read than this new implementation. How
about:

base::win::RegKey config_key;
if (config_key.Open(HKEY_CURRENT_USER, key_name_.c_str(),
                    KEY_SET_VALUE) == ERROR_SUCCESS &&
    config_key.WriteValue(kReadyModeStateValue, &value, sizeof(value),
                          REG_QWORD) == ERROR_SUCCESS) {
 return true;
}

DLOG(ERROR) << "Failed to open or write to registry key " << key_name_
            << " and value " << kReadyModeStateValue;
            << " and value " << kReadyModeStateValue << " Error: "
            << result;
return false;

http://codereview.chromium.org/6090006/diff/30005/chrome_frame/utils.cc
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/6090006/diff/30005/chrome_frame/utils.cc#newco...
chrome_frame/utils.cc:237: success = result == ERROR_SUCCESS;
please add parenthesis around (result == ERROR_SUCCCESS)

http://codereview.chromium.org/6090006/diff/30005/chrome_frame/utils.cc#newco...
chrome_frame/utils.cc:681: KEY_QUERY_VALUE) == ERROR_SUCCESS) {
Summary comment, I like this style better than using a temporary.

http://codereview.chromium.org/6090006/diff/30005/net/base/platform_mime_util...
File net/base/platform_mime_util_win.cc (right):

http://codereview.chromium.org/6090006/diff/30005/net/base/platform_mime_util...
net/base/platform_mime_util_win.cc:37: if (ret != ERROR_SUCCESS) {
nit: how about

if (extension_key.Open(HKEY_CLASSES_ROOT, key.c_str(),
                       KEY_READ) != ERROR_SUCCESS ||
    extension_key.ReadValue(L"Extension", ext) != ERROR_SUCCESS) {
  return false;
}

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

Just a few comments this time around.  Lookin' great overall.

http://codereview.chromium.org/6090006/diff/141066/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_ie_toolbar_import_win.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_ie_toolbar_import_win.cc:60: LONG result =
key->ReadValue(value_name, NULL, &data_size, &data_type);
On 2011/01/12 04:11:23, amit wrote:
> On 2011/01/10 20:42:28, brettw wrote:
> > 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.
> 
> 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
> :)

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.

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/dele...
File chrome/installer/util/delete_reg_value_work_item.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/dele...
chrome/installer/util/delete_reg_value_work_item.cc:46: if
(!key.ValueExists(value_name_.c_str())) {
On 2011/01/11 03:51:30, grt wrote:
> It's less work and less code to remove this call to ValueExists and hande
> ERROR_FILE_NOT_FOUND from the later calls to ReadValue*, no?

Ping.

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
File chrome/installer/util/google_update_settings.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:260: key.Close();
Remove

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:265: if (result !=
ERROR_SUCCESS)
On 2011/01/12 04:11:23, amit wrote:
> On 2011/01/11 03:51:30, grt wrote:
> > result == ERROR_SUCCESS
> Are you sure? As per the old code, key.CreateKey will execute only if key.Open
> fails.

I'm pretty sure CreateKey executes only if Open succeeds (unless the demons are
talking to me again).  My read of the old code is "if we can't Open the
ClientState key or if we can't create the product GUID key beneath it, log and
bail."

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:266:
key.CreateKey(product_guid.c_str(), KEY_ALL_ACCESS);
On 2011/01/11 03:51:30, grt wrote:
> result = key.CreateKey(...)

Ping.

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:270: key.Close();
Remove

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/prod...
File chrome/installer/util/product.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/prod...
chrome/installer/util/product.cc:157: << " error: " << result;
On 2011/01/12 04:11:23, amit wrote:
> On 2011/01/11 03:51:30, grt wrote:
> > com::LogWe(result)  (i love this!)
> com::LogWe is cool and I have changed to it in CEEE. However, I am not sure if
> we want to make installer depend on a CEEE lib. Maybe a first step is to
factor
> that out in a common lib elsewhere?

Yeah; I didn't notice that it was in a CEEE lib.

http://codereview.chromium.org/6090006/diff/30005/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_provider_delegate_win.cc
(right):

http://codereview.chromium.org/6090006/diff/30005/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider_delegate_win.cc:129: *result
= local_result != 0;
Same comment as before: only modify *result when ret != false.  (Consistent with
your "I think it's not proper for an API to return failure but cause side
effects in the return arg." statement, which I'm down with.)

[brettw, 2011-01-12 23:45:20]

If you're planning on removing your GONG hack, I don't see how you can say that
your new version is more robust than saving the LONG value somewhere, when the
most obvious calling code is wrong:
  if (!foo.GetValue())
This affects hundreds of callers rather than just a few internal functions.

[amit, 2011-01-13 00:01:52]

Seems like more people are in favor of eliminating a temporary in 'if'
conditions, so I'll remove it. :)
In some places I have added it to DLOGs to print error codes (also, you can
easily inspect return value while debugging that code) such as here:
http://codereview.chromium.org/6090006/diff/30005/chrome_frame/crash_reportin...
   So unless you think they are necessary, I'll remove those additions as
well.

On Wed, Jan 12, 2011 at 7:01 AM, <robertshield@chromium.org> wrote:

> Mostly style nits, one thing that looks like a regression (|| instead of
> &&).
>
> In general, I find adding temporary vars and then sometimes breaking up
> conditions into multiple if statements a bit harder to read (I have to
> think
> about what they're doing slightly longer). I didn't come across many
> egregious
> cases here.
>
>
>
> http://codereview.chromium.org/6090006/diff/30005/chrome_frame/chrome_tab.cc
> File chrome_frame/chrome_tab.cc (right):
>
>
>
http://codereview.chromium.org/6090006/diff/30005/chrome_frame/chrome_tab.cc#...
> chrome_frame/chrome_tab.cc:323: LONG ret = run_once.Create(hive,
> kRunOnce, KEY_READ | KEY_WRITE);
> nit:
> if (run_once.Create(hive, kRunOnce, KEY_READ | KEY_WRITE) ==
> ERROR_SUCCESS) {
> would fit on one line, allowing you to do away with ret, at least at
> this scope level.
>
>
>
http://codereview.chromium.org/6090006/diff/30005/chrome_frame/chrome_tab.cc#...
> chrome_frame/chrome_tab.cc:748: return (result2 == ERROR_SUCCESS) ||
> (result2 == ERROR_SUCCESS);
> The || should be an &&: previously both writes/deletes had to succeed
> for this to return true, now only one of them has to succeed.
>
>
>
http://codereview.chromium.org/6090006/diff/30005/chrome_frame/crash_reportin...
> File chrome_frame/crash_reporting/crash_metrics.cc (right):
>
>
>
http://codereview.chromium.org/6090006/diff/30005/chrome_frame/crash_reportin...
> chrome_frame/crash_reporting/crash_metrics.cc:34: KEY_SET_VALUE);
> nit: the temporary here makes this harder to read imho, consider
> formatting it similarly to GetMetric() a few lines down.
>
>
>
http://codereview.chromium.org/6090006/diff/30005/chrome_frame/ready_mode/int...
> File chrome_frame/ready_mode/internal/registry_ready_mode_state.cc
> (right):
>
>
>
http://codereview.chromium.org/6090006/diff/30005/chrome_frame/ready_mode/int...
> chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:111: }
> More nitty whining:
> I found the original code easier to read than this new implementation.
> How about:
>
> base::win::RegKey config_key;
> if (config_key.Open(HKEY_CURRENT_USER, key_name_.c_str(),
>                    KEY_SET_VALUE) == ERROR_SUCCESS &&
>    config_key.WriteValue(kReadyModeStateValue, &value, sizeof(value),
>                          REG_QWORD) == ERROR_SUCCESS) {
>  return true;
> }
>
> DLOG(ERROR) << "Failed to open or write to registry key " << key_name_
>            << " and value " << kReadyModeStateValue;
>            << " and value " << kReadyModeStateValue << " Error: "
>            << result;
> return false;
>
> http://codereview.chromium.org/6090006/diff/30005/chrome_frame/utils.cc
> File chrome_frame/utils.cc (right):
>
>
>
http://codereview.chromium.org/6090006/diff/30005/chrome_frame/utils.cc#newco...
> chrome_frame/utils.cc:237: success = result == ERROR_SUCCESS;
> please add parenthesis around (result == ERROR_SUCCCESS)
>
>
>
http://codereview.chromium.org/6090006/diff/30005/chrome_frame/utils.cc#newco...
> chrome_frame/utils.cc:681: KEY_QUERY_VALUE) == ERROR_SUCCESS) {
> Summary comment, I like this style better than using a temporary.
>
>
>
http://codereview.chromium.org/6090006/diff/30005/net/base/platform_mime_util...
> File net/base/platform_mime_util_win.cc (right):
>
>
>
http://codereview.chromium.org/6090006/diff/30005/net/base/platform_mime_util...
> net/base/platform_mime_util_win.cc:37: if (ret != ERROR_SUCCESS) {
> nit: how about
>
> if (extension_key.Open(HKEY_CLASSES_ROOT, key.c_str(),
>                       KEY_READ) != ERROR_SUCCESS ||
>    extension_key.ReadValue(L"Extension", ext) != ERROR_SUCCESS) {
>  return false;
>
> }
>
> http://codereview.chromium.org/6090006/
>

[amit, 2011-01-13 00:37:12]

On Wed, Jan 12, 2011 at 3:45 PM, <brettw@chromium.org> wrote:

> If you're planning on removing your GONG hack, I don't see how you can say
> that
> your new version is more robust than saving the LONG value somewhere, when
> the
> most obvious calling code is wrong:
>  if (!foo.GetValue())
> This affects hundreds of callers rather than just a few internal functions.
>
> I am not sure if I follow you correctly.  Do you mean that it's better to
offer an API such as:

BOOL ReadFile(...)

so that the caller could just write:

if (!ReadFile(...) {

  DWORD error = GetLastError(); ...

}

vs:

NTSTATUS ZwReadFile(...);

with the usage as:

NTSTATUS status = ZwReadFile(...);

if (!NT_SUCCESS(status) {

...

}



> http://codereview.chromium.org/6090006/
>

[brettw, 2011-01-13 00:50:17]

On 2011/01/13 00:37:12, amit wrote:
> On Wed, Jan 12, 2011 at 3:45 PM, <mailto:brettw@chromium.org> wrote:
> 
> > If you're planning on removing your GONG hack, I don't see how you can say
> > that
> > your new version is more robust than saving the LONG value somewhere, when
> > the
> > most obvious calling code is wrong:
> >  if (!foo.GetValue())
> > This affects hundreds of callers rather than just a few internal functions.
> >
> > I am not sure if I follow you correctly.  Do you mean that it's better to
> offer an API such as:
> 
> BOOL ReadFile(...)
> 
> so that the caller could just write:
> 
> if (!ReadFile(...) {
> 
>   DWORD error = GetLastError(); ...
> 
> }
> 
> vs:
> 
> NTSTATUS ZwReadFile(...);
> 
> with the usage as:
> 
> NTSTATUS status = ZwReadFile(...);
> 
> if (!NT_SUCCESS(status) {
> 
> ...
> 
> }

Yes.

[cpu_(ooo_6.6-7.5), 2011-01-13 01:36:35]

I think the API should reflect the way it is used. Given the maturity of chrome
and that 99% of the callers only care success or failure, I think bool is the
superior choice here.

Not fond of CatchBoolChecks in its current form either.

There is also my realization in the past 4 months that people with 0 windows
experience are writing windows code, possibly the trybots embolden people that
otherwise will seek help of a windows experienced peer to write the windows side
of his pet feature. The nasty buts from the confusion between 0 and
INVALID_HANDLE value is one example.


Anyhow done with the rant, but I've come with options:

1- In the case you actually need the extra functionality, call windows directly.
Trying to make base be all for everybody leads to, for example, our
::CreateProcess wrapper which makes me sad every time I see it.

2- Add the last_error_ member variable and a method to get it.

3- a little class with the opposite function of CatchBoolChecks, it acts like a
bool when used in an if() but also has a member ActualError() so in the case you
care you have to say 

GOG g = key.F()
if (!g) {
  if (g.ActualError() == xxx)
}


My vote goes for #1.






On 2011/01/13 00:50:17, brettw wrote:
> On 2011/01/13 00:37:12, amit wrote:
> > On Wed, Jan 12, 2011 at 3:45 PM, <mailto:brettw@chromium.org> wrote:
> > 
> > > If you're planning on removing your GONG hack, I don't see how you can say
> > > that
> > > your new version is more robust than saving the LONG value somewhere, when
> > > the
> > > most obvious calling code is wrong:
> > >  if (!foo.GetValue())
> > > This affects hundreds of callers rather than just a few internal
functions.
> > >
> > > I am not sure if I follow you correctly.  Do you mean that it's better to
> > offer an API such as:
> > 
> > BOOL ReadFile(...)
> > 
> > so that the caller could just write:
> > 
> > if (!ReadFile(...) {
> > 
> >   DWORD error = GetLastError(); ...
> > 
> > }
> > 
> > vs:
> > 
> > NTSTATUS ZwReadFile(...);
> > 
> > with the usage as:
> > 
> > NTSTATUS status = ZwReadFile(...);
> > 
> > if (!NT_SUCCESS(status) {
> > 
> > ...
> > 
> > }
> 
> Yes.

[Sigurður Ásgeirsson, 2011-01-13 15:05:02]

On Wed, Jan 12, 2011 at 8:36 PM, <cpu@chromium.org> wrote:

> I think the API should reflect the way it is used. Given the maturity of
> chrome
> and that 99% of the callers only care success or failure, I think bool is
> the
> superior choice here.
>
> Not fond of CatchBoolChecks in its current form either.
>
> There is also my realization in the past 4 months that people with 0
> windows
> experience are writing windows code, possibly the trybots embolden people
> that
> otherwise will seek help of a windows experienced peer to write the windows
> side
> of his pet feature. The nasty buts from the confusion between 0 and
> INVALID_HANDLE value is one example.
>
>
> Anyhow done with the rant, but I've come with options:
>
> 1- In the case you actually need the extra functionality, call windows
> directly.
> Trying to make base be all for everybody leads to, for example, our
> ::CreateProcess wrapper which makes me sad every time I see it.
>
> The argument for using wrappers consistently, as opposed to going straight
to the OSs primitives, is that with wrappers you have a choke point for
implementing additional services or testing conditions/invariants. The IO
checks are a good example of this.



> 2- Add the last_error_ member variable and a method to get it.
>
>
How about an overload for each function that returns the error in an out
variable? That way we can get the errors for only the callers that are
interested, without:
- changing the calling convention for those that don't care about the
errors,
- compromising thread safety,
- additional state for all callers,
- making the code more opaque or fragile than it is.


> 3- a little class with the opposite function of CatchBoolChecks, it acts
> like a
> bool when used in an if() but also has a member ActualError() so in the
> case you
> care you have to say
>
> GOG g = key.F()
> if (!g) {
>  if (g.ActualError() == xxx)
> }
>
>
> My vote goes for #1.
>
>
>
>
>
>
>
> On 2011/01/13 00:50:17, brettw wrote:
>
>> On 2011/01/13 00:37:12, amit wrote:
>> > On Wed, Jan 12, 2011 at 3:45 PM, <mailto:brettw@chromium.org> wrote:
>> >
>> > > If you're planning on removing your GONG hack, I don't see how you can
>> say
>> > > that
>> > > your new version is more robust than saving the LONG value somewhere,
>> when
>> > > the
>> > > most obvious calling code is wrong:
>> > >  if (!foo.GetValue())
>> > > This affects hundreds of callers rather than just a few internal
>>
> functions.
>
>> > >
>> > > I am not sure if I follow you correctly.  Do you mean that it's better
>> to
>> > offer an API such as:
>> >
>> > BOOL ReadFile(...)
>> >
>> > so that the caller could just write:
>> >
>> > if (!ReadFile(...) {
>> >
>> >   DWORD error = GetLastError(); ...
>> >
>> > }
>> >
>> > vs:
>> >
>> > NTSTATUS ZwReadFile(...);
>> >
>> > with the usage as:
>> >
>> > NTSTATUS status = ZwReadFile(...);
>> >
>> > if (!NT_SUCCESS(status) {
>> >
>> > ...
>> >
>> > }
>>
>
>  Yes.
>>
>
>
>
> http://codereview.chromium.org/6090006/
>

[grt (UTC plus 2), 2011-01-13 16:01:16]

On Thu, Jan 13, 2011 at 10:04 AM, Sigurður Ásgeirsson <siggi@chromium.org>wrote:

>
>
>> 2- Add the last_error_ member variable and a method to get it.
>>
>>
> How about an overload for each function that returns the error in an out
> variable?
>
>
It's also worth considering which errors are handled in practice.  If it's
the case that we really only care to do special processing when something
isn't present (e.g., ERROR_FILE_NOT_FOUND from RegQueryValueEx), we could
add an optional out variable that provides only this information.  Callers
wouldn't need to concern themselves with winerror.h.

I'm not sure if would make the API prettier or uglier...

[brettw, 2011-01-13 17:09:21]

On Thu, Jan 13, 2011 at 7:04 AM, Sigurður Ásgeirsson <siggi@chromium.org> wrote:
> How about an overload for each function that returns the error in an out
> variable? That way we can get the errors for only the callers that are
> interested, without:
> - changing the calling convention for those that don't care about the
> errors,
> - compromising thread safety,
> - additional state for all callers,
> - making the code more opaque or fragile than it is.

I at least like this approach better than returning a LONG, but I'm
not really a fan.

I still reject that RegKey should be threadsafe so I don't consider
that an advantage.

And I still don't see why having the functions store their arguments
in a last_error_ member is considered fragile. It's not like we're
constantly adding new functions to this class. In fact, there have
been ZERO additions of new functions since this code was added to the
public repository nearly 3 years ago. And all of the current
Read/Write functions are copied and pasted versions of each other,
which I would presume would continue even if neither of a hypothetical
code and reviewer knew enough to notice that they should save the last
return value in a hypothetical new function. I have no concerns about
this approach adding fragility or long-term maintenance problems.

Google style prohibits arguments with default values. We have a
history of using overloaded function in some places to avoid optional
arguments. But this ends up being pretty stupid since you get code
like this (which I assume is what we'll end up with in your proposal):

int Foo(a, b) {
  return Foo(a, b, NULL);
}

int Foo(a, b, c) {
 ...
}

which totally misses the point of the rule against optional arguments.
In fact, tfarina removed a bunch of default arguments from the RegKey
class a few months ago, so it seems that adding back default arguments
in some form would be backsliding.

Brett

[Sigurður Ásgeirsson, 2011-01-13 18:13:19]

On Thu, Jan 13, 2011 at 12:09 PM, Brett Wilson <brettw@chromium.org> wrote:

> On Thu, Jan 13, 2011 at 7:04 AM, Sigurður Ásgeirsson <siggi@chromium.org>
> wrote:
> > How about an overload for each function that returns the error in an out
> > variable? That way we can get the errors for only the callers that are
> > interested, without:
> > - changing the calling convention for those that don't care about the
> > errors,
> > - compromising thread safety,
> > - additional state for all callers,
> > - making the code more opaque or fragile than it is.
>
> I at least like this approach better than returning a LONG, but I'm
> not really a fan.
>
> I still reject that RegKey should be threadsafe so I don't consider
> that an advantage.
>
> And I still don't see why having the functions store their arguments
> in a last_error_ member is considered fragile.


All forms of this tend to be fragile against maintenance, think e.g. errno,
GetLastError, Set/GetErrorInfo etc. Eventually something that perturbs the
state of the error value slips in between the invocation that causes the
error and the code that uses it, and as the error handing is typically an
exceptional occurrence, it's always a mess to track this stuff down.

That being said, I don't have a strong opinion either way.


> It's not like we're
> constantly adding new functions to this class. In fact, there have
> been ZERO additions of new functions since this code was added to the
> public repository nearly 3 years ago. And all of the current
> Read/Write functions are copied and pasted versions of each other,
> which I would presume would continue even if neither of a hypothetical
> code and reviewer knew enough to notice that they should save the last
> return value in a hypothetical new function. I have no concerns about
> this approach adding fragility or long-term maintenance problems.
>
> Google style prohibits arguments with default values. We have a
> history of using overloaded function in some places to avoid optional
> arguments. But this ends up being pretty stupid since you get code
> like this (which I assume is what we'll end up with in your proposal):
>
> int Foo(a, b) {
>  return Foo(a, b, NULL);
> }
>
> int Foo(a, b, c) {
>  ...
> }
>
> which totally misses the point of the rule against optional arguments.
> In fact, tfarina removed a bunch of default arguments from the RegKey
> class a few months ago, so it seems that adding back default arguments
> in some form would be backsliding.
>
> Brett
>

[sanjeevr, 2011-01-13 18:21:25]

The abhorrence of the GetLastError paradigm comes from years of dealing with
issues caused by this on Windows. Uses a tls-value to store the error
returned by an API was a mistake that Microsoft made (their senior
developers have pretty much said so in conferences like the PDC). Their
newer APIs do not follow this paradigm. The brittleness comes from another
API inadvertently overwriting the "last error" (like your API fails, you
call CloseHandle which overwrites the last error and now you have lost the
error code - this gets especially tricky when the code calling CloseHandle
is in a d'tor of some object which you don't have direct control over). You
need to also make sure that setting this code is the very last thing you do
in from every method call.

Knowing the reason why something failed (or in some cases why something
succeeded) is important in many cases in Windows system calls. CreateMutex
will succeed even if a mutex by that name already existed before, checking
the error code is the only way to tell the difference. Failing to open a
registry key because it did not exist can need entirely different handling
from failing to open it because you do not have access.

Using a return value or an out-param is the only decent non-brittle way.

Amit, could you not have the bool operator be public and return true if the
value is zero. That way the if (!foo()) should work as well as LONG error =
foo();

On Thu, Jan 13, 2011 at 9:09 AM, Brett Wilson <brettw@chromium.org> wrote:

> On Thu, Jan 13, 2011 at 7:04 AM, Sigurður Ásgeirsson <siggi@chromium.org>
> wrote:
> > How about an overload for each function that returns the error in an out
> > variable? That way we can get the errors for only the callers that are
> > interested, without:
> > - changing the calling convention for those that don't care about the
> > errors,
> > - compromising thread safety,
> > - additional state for all callers,
> > - making the code more opaque or fragile than it is.
>
> I at least like this approach better than returning a LONG, but I'm
> not really a fan.
>
> I still reject that RegKey should be threadsafe so I don't consider
> that an advantage.
>
> And I still don't see why having the functions store their arguments
> in a last_error_ member is considered fragile. It's not like we're
> constantly adding new functions to this class. In fact, there have
> been ZERO additions of new functions since this code was added to the
> public repository nearly 3 years ago. And all of the current
> Read/Write functions are copied and pasted versions of each other,
> which I would presume would continue even if neither of a hypothetical
> code and reviewer knew enough to notice that they should save the last
> return value in a hypothetical new function. I have no concerns about
> this approach adding fragility or long-term maintenance problems.
>
> Google style prohibits arguments with default values. We have a
> history of using overloaded function in some places to avoid optional
> arguments. But this ends up being pretty stupid since you get code
> like this (which I assume is what we'll end up with in your proposal):
>
> int Foo(a, b) {
>  return Foo(a, b, NULL);
> }
>
> int Foo(a, b, c) {
>  ...
> }
>
> which totally misses the point of the rule against optional arguments.
> In fact, tfarina removed a bunch of default arguments from the RegKey
> class a few months ago, so it seems that adding back default arguments
> in some form would be backsliding.
>
> Brett
>

[tommi (sloooow) - chröme, 2011-01-13 19:44:36]

Thanks for doing this Amit.

One nit I have is that GONG doesn't sound very descriptive.  Perhaps RegResult?

As far as the bikeshedding goes, I think that a simple wrapper class such as
this one should be at least as useful as the API it wraps.

Hiding the detailed error information like RegKey has been doing, is a design
flaw.  It can cause bugs (e.g. delete_reg_value), odd workarounds like opening
the same key more than once, and encourages the programmer to not bother about
proper error handling as is often needed in the case of registry access.  To
make it more clear, RegKey has _prevented_ proper error handling in the past, so
this change is very welcome. :-)

*awaits the flaming poo*

[amit, 2011-01-13 21:37:51]

Siggi, Sanjeev and Tommi have given enough good reasons why GetLastError()
is not a recommended pattern.  IMO, the drawbacks clearly outweigh gain in
terms of readability.


On Thu, Jan 13, 2011 at 11:44 AM, <tommi@chromium.org> wrote:

> Thanks for doing this Amit.
>
> One nit I have is that GONG doesn't sound very descriptive.  Perhaps
> RegResult?
>
> As far as the bikeshedding goes, I think that a simple wrapper class such
> as
> this one should be at least as useful as the API it wraps.
>
> Hiding the detailed error information like RegKey has been doing, is a
> design
> flaw.  It can cause bugs (e.g. delete_reg_value), odd workarounds like
> opening
> the same key more than once, and encourages the programmer to not bother
> about
> proper error handling as is often needed in the case of registry access.
>  To
> make it more clear, RegKey has _prevented_ proper error handling in the
> past, so
> this change is very welcome. :-)
>
And for exactly this reason, GONG is intended as a temporary hack and I am
skeptical of formalizing it. Think of it this way: do we want formalize GONG
pattern for any helper returning error or status?  For e.g.
 PreamblePatcher::Patch(...) would return GONG<SideStepError> instead of
just plain SideStepError?


> *awaits the flaming poo*
>
>
> http://codereview.chromium.org/6090006/
>

[amit, 2011-01-13 22:48:13]

On Thu, Jan 13, 2011 at 1:37 PM, Amit Joshi <amit@chromium.org> wrote:

> Siggi, Sanjeev and Tommi have given enough good reasons why GetLastError()
> is not a recommended pattern.  IMO, the drawbacks clearly outweigh gain in
> terms of readability.

On the readability note would the following macro make it more readable?
#define REG_SUCCESS(r)      ((r) == ERROR_SUCCESS)


>
>
> On Thu, Jan 13, 2011 at 11:44 AM, <tommi@chromium.org> wrote:
>
>> Thanks for doing this Amit.
>>
>> One nit I have is that GONG doesn't sound very descriptive.  Perhaps
>> RegResult?
>>
>> As far as the bikeshedding goes, I think that a simple wrapper class such
>> as
>> this one should be at least as useful as the API it wraps.
>>
>> Hiding the detailed error information like RegKey has been doing, is a
>> design
>> flaw.  It can cause bugs (e.g. delete_reg_value), odd workarounds like
>> opening
>> the same key more than once, and encourages the programmer to not bother
>> about
>> proper error handling as is often needed in the case of registry access.
>>  To
>> make it more clear, RegKey has _prevented_ proper error handling in the
>> past, so
>> this change is very welcome. :-)
>>
> And for exactly this reason, GONG is intended as a temporary hack and I am
> skeptical of formalizing it. Think of it this way: do we want formalize GONG
> pattern for any helper returning error or status?  For e.g.
>  PreamblePatcher::Patch(...) would return GONG<SideStepError> instead of
> just plain SideStepError?
>
>
>> *awaits the flaming poo*
>>
>>
>> http://codereview.chromium.org/6090006/
>>
>
>

[brettw, 2011-01-13 23:04:09]

On Thu, Jan 13, 2011 at 11:44 AM,  <tommi@chromium.org> wrote:
> Thanks for doing this Amit.
>
> One nit I have is that GONG doesn't sound very descriptive.  Perhaps
> RegResult?
>
> As far as the bikeshedding goes, I think that a simple wrapper class such as
> this one should be at least as useful as the API it wraps.
>
> Hiding the detailed error information like RegKey has been doing, is a
> design
> flaw.  It can cause bugs (e.g. delete_reg_value), odd workarounds like
> opening
> the same key more than once, and encourages the programmer to not bother
> about
> proper error handling as is often needed in the case of registry access.  To
> make it more clear, RegKey has _prevented_ proper error handling in the
> past, so
> this change is very welcome. :-)

Can you elaborate on these cases? I didn't really feel like I got a
clear understanding above about what should we do differently in
different error cases.

Brett

[tommi (sloooow) - chröme, 2011-01-13 23:06:01]

A very simple example is RegDeleteValue.  It can fail because the value
doesn't exist, insufficient access, parent key doesn't exist etc.  You
typically do not want to handle each case the same.

On Thu, Jan 13, 2011 at 6:04 PM, Brett Wilson <brettw@chromium.org> wrote:

> On Thu, Jan 13, 2011 at 11:44 AM,  <tommi@chromium.org> wrote:
> > Thanks for doing this Amit.
> >
> > One nit I have is that GONG doesn't sound very descriptive.  Perhaps
> > RegResult?
> >
> > As far as the bikeshedding goes, I think that a simple wrapper class such
> as
> > this one should be at least as useful as the API it wraps.
> >
> > Hiding the detailed error information like RegKey has been doing, is a
> > design
> > flaw.  It can cause bugs (e.g. delete_reg_value), odd workarounds like
> > opening
> > the same key more than once, and encourages the programmer to not bother
> > about
> > proper error handling as is often needed in the case of registry access.
>  To
> > make it more clear, RegKey has _prevented_ proper error handling in the
> > past, so
> > this change is very welcome. :-)
>
> Can you elaborate on these cases? I didn't really feel like I got a
> clear understanding above about what should we do differently in
> different error cases.
>
> Brett
>

[brettw, 2011-01-13 23:09:00]

On Thu, Jan 13, 2011 at 3:05 PM, Tommi <tommi@chromium.org> wrote:
> A very simple example is RegDeleteValue.  It can fail because the value
> doesn't exist, insufficient access, parent key doesn't exist etc.  You
> typically do not want to handle each case the same.

Sorry about my ignorance. Can you find some code that does this and
tell me what should happen in the different cases? It seems from
looking at this kind of thing in the past is that the two approaches
are either ignore the error or give up.

Brett

[amit, 2011-01-13 23:45:42]

On Thu, Jan 13, 2011 at 3:08 PM, Brett Wilson <brettw@chromium.org> wrote:

> On Thu, Jan 13, 2011 at 3:05 PM, Tommi <tommi@chromium.org> wrote:
> > A very simple example is RegDeleteValue.  It can fail because the value
> > doesn't exist, insufficient access, parent key doesn't exist etc.  You
> > typically do not want to handle each case the same.
>
> Sorry about my ignorance. Can you find some code that does this and
> tell me what should happen in the different cases? It seems from
> looking at this kind of thing in the past is that the two approaches
> are either ignore the error or give up.
>

For e.g. I just merged with this code.
Original:

  HKEY roots[] = { HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE };
  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;
        *exists = true;
        return true;
      }
    }
  }

Merged:

  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;
    result = config_key.Open(HKEY_CURRENT_USER, key_name_.c_str(),
                             KEY_QUERY_VALUE);
    if (result == ERROR_SUCCESS) {
      result = config_key.ReadValueInt64(kReadyModeStateValue, 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;
      }
    }
  }



>
> Brett
>

[cpu_(ooo_6.6-7.5), 2011-01-14 02:22:45]

Note that the method HKEY Handle() is implemented already we we could rewrite
amit's example without much trouble to do what he needs. Yes, I am still rooting
for my option #1.

The disadvantage of the GetLastError() is clear to me. The hope is that because
we do this in the wrapper we would not forget, basically the in the code review
pattern should be:

last_error_ = ::RegXXXX(...);

If we follow this in all the methods we should be ok. We wont be using TLS or
abusing SetLastError(). So yes, my second vote goes for last_error_.

I know we are going in circles now but I think is a good think we get an
agreement what the windows wrappers in general are going to hide (or to expose)
because the registry wrappers are just but one small class and the outcome of
this pretty much opens the door to N^2 more reviews for all the other windows
wrappers to start exposing (leaking) the return codes.



On 2011/01/13 23:45:42, amit wrote:
> On Thu, Jan 13, 2011 at 3:08 PM, Brett Wilson <mailto:brettw@chromium.org>
wrote:
> 
> > On Thu, Jan 13, 2011 at 3:05 PM, Tommi <mailto:tommi@chromium.org> wrote:
> > > A very simple example is RegDeleteValue.  It can fail because the value
> > > doesn't exist, insufficient access, parent key doesn't exist etc.  You
> > > typically do not want to handle each case the same.
> >
> > Sorry about my ignorance. Can you find some code that does this and
> > tell me what should happen in the different cases? It seems from
> > looking at this kind of thing in the past is that the two approaches
> > are either ignore the error or give up.
> >
> 
> For e.g. I just merged with this code.
> Original:
> 
>   HKEY roots[] = { HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE };
>   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;
>         *exists = true;
>         return true;
>       }
>     }
>   }
> 
> Merged:
> 
>   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;
>     result = config_key.Open(HKEY_CURRENT_USER, key_name_.c_str(),
>                              KEY_QUERY_VALUE);
>     if (result == ERROR_SUCCESS) {
>       result = config_key.ReadValueInt64(kReadyModeStateValue, 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;
>       }
>     }
>   }
> 
> 
> 
> >
> > Brett
> >

[amit, 2011-01-14 04:24:27]

Option #1 negates the benefits of a wrapper to a large extent, you can
notice the difference between the original (where ReadInt64 was rolled out
inline) and merged. Rolling out ReadValue for wstrings anywhere you need a
return value is not going to be pretty.
Option #2: GetLastError() pattern:  Tommi and others have already laid out
drawbacks.

Now that the disadvantages are clear, lets say we go with Option #1 or #2
what's the upside?
Only thing I can think of is that an 'if' expression is less ugly to look
at.  That's also questionable, because majority of the RegKey users are
already familiar with the Registry error codes. In fact, it is counter
intuitive for me that RegKey wrappers return a bool while underlying API
returns an error code.  A 'SUCCESS' macro can easily fix readability to some
extent.
Also, I am dubious about protecting someone with zero windows knowledge and
intends to use this API without understanding it. I don't think any
framework can protect against that.

I think benefits of returning error clearly outweigh the drawbacks and yes
feels like deja vu talking about it...
wait, did I just say that? :)

On Thu, Jan 13, 2011 at 6:22 PM, <cpu@chromium.org> wrote:

> Note that the method HKEY Handle() is implemented already we we could
> rewrite
> amit's example without much trouble to do what he needs. Yes, I am still
> rooting
> for my option #1.
>
> The disadvantage of the GetLastError() is clear to me. The hope is that
> because
> we do this in the wrapper we would not forget, basically the in the code
> review
> pattern should be:
>
> last_error_ = ::RegXXXX(...);
>
> If we follow this in all the methods we should be ok. We wont be using TLS
> or
> abusing SetLastError(). So yes, my second vote goes for last_error_.
>
> I know we are going in circles now but I think is a good think we get an
> agreement what the windows wrappers in general are going to hide (or to
> expose)
> because the registry wrappers are just but one small class and the outcome
> of
> this pretty much opens the door to N^2 more reviews for all the other
> windows
> wrappers to start exposing (leaking) the return codes.
>
>
>
> On 2011/01/13 23:45:42, amit wrote:
>
>> On Thu, Jan 13, 2011 at 3:08 PM, Brett Wilson <mailto:brettw@chromium.org
>> >
>>
> wrote:
>
>
>  > On Thu, Jan 13, 2011 at 3:05 PM, Tommi <mailto:tommi@chromium.org>
>> wrote:
>> > > A very simple example is RegDeleteValue.  It can fail because the
>> value
>> > > doesn't exist, insufficient access, parent key doesn't exist etc.  You
>> > > typically do not want to handle each case the same.
>> >
>> > Sorry about my ignorance. Can you find some code that does this and
>> > tell me what should happen in the different cases? It seems from
>> > looking at this kind of thing in the past is that the two approaches
>> > are either ignore the error or give up.
>> >
>>
>
>  For e.g. I just merged with this code.
>> Original:
>>
>
>    HKEY roots[] = { HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE };
>>   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;
>>         *exists = true;
>>         return true;
>>       }
>>     }
>>   }
>>
>
>  Merged:
>>
>
>    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;
>>     result = config_key.Open(HKEY_CURRENT_USER, key_name_.c_str(),
>>                              KEY_QUERY_VALUE);
>>     if (result == ERROR_SUCCESS) {
>>       result = config_key.ReadValueInt64(kReadyModeStateValue, 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;
>>       }
>>     }
>>   }
>>
>
>
>
>  >
>> > Brett
>> >
>>
>
>
>
> http://codereview.chromium.org/6090006/
>

[amit, 2011-01-14 05:23:33]

Updated, please continue.

Other changes:
- Cleanup DeleteRegValueWorkItem and SetRegValueWorkItem
- Removed 'type' arg from DeleteWorkItem
- Added to unit test cases for RegKey

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/dele...
File chrome/installer/util/delete_reg_value_work_item.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/dele...
chrome/installer/util/delete_reg_value_work_item.cc:46: if
(!key.ValueExists(value_name_.c_str())) {
On 2011/01/12 15:06:38, grt wrote:
> On 2011/01/11 03:51:30, grt wrote:
> > It's less work and less code to remove this call to ValueExists and hande
> > ERROR_FILE_NOT_FOUND from the later calls to ReadValue*, no?
> 
> Ping.

Sorry, missed it in the first iteration. Now made it even better :)

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
File chrome/installer/util/google_update_settings.cc (right):

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:260: key.Close();
On 2011/01/12 15:06:38, grt wrote:
> Remove

Done.

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:265: if (result !=
ERROR_SUCCESS)
On 2011/01/12 15:06:38, grt wrote:
> On 2011/01/12 04:11:23, amit wrote:
> > On 2011/01/11 03:51:30, grt wrote:
> > > result == ERROR_SUCCESS
> > Are you sure? As per the old code, key.CreateKey will execute only if
key.Open
> > fails.
> 
> I'm pretty sure CreateKey executes only if Open succeeds (unless the demons
are
> talking to me again).  My read of the old code is "if we can't Open the
> ClientState key or if we can't create the product GUID key beneath it, log and
> bail."

Done.

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:266:
key.CreateKey(product_guid.c_str(), KEY_ALL_ACCESS);
On 2011/01/12 15:06:38, grt wrote:
> On 2011/01/11 03:51:30, grt wrote:
> > result = key.CreateKey(...)
> 
> Ping.

Done.

http://codereview.chromium.org/6090006/diff/141066/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:270: key.Close();
On 2011/01/12 15:06:38, grt wrote:
> Remove

Done.

http://codereview.chromium.org/6090006/diff/30005/base/win/registry.h
File base/win/registry.h (right):

http://codereview.chromium.org/6090006/diff/30005/base/win/registry.h#newcode28
base/win/registry.h:28: operator bool () {return false;}
On 2011/01/12 14:04:19, Ruðrugis wrote:
> nit: ws around {}

Done.

http://codereview.chromium.org/6090006/diff/30005/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_provider_delegate_win.cc
(right):

http://codereview.chromium.org/6090006/diff/30005/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider_delegate_win.cc:129: *result
= local_result != 0;
On 2011/01/12 15:06:38, grt wrote:
> Same comment as before: only modify *result when ret != false.  (Consistent
with
> your "I think it's not proper for an API to return failure but cause side
> effects in the return arg." statement, which I'm down with.)

Good catch, done.

http://codereview.chromium.org/6090006/diff/30005/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/6090006/diff/30005/chrome_frame/chrome_tab.cc#...
chrome_frame/chrome_tab.cc:323: LONG ret = run_once.Create(hive, kRunOnce,
KEY_READ | KEY_WRITE);
On 2011/01/12 15:01:34, robertshield wrote:
> nit: 
> if (run_once.Create(hive, kRunOnce, KEY_READ | KEY_WRITE) == ERROR_SUCCESS) {
> would fit on one line, allowing you to do away with ret, at least at this
scope
> level.

Actually, it does not fit on one line but that's not the point here.
SetupRunOnce now returns a proper return code in case of failure.

http://codereview.chromium.org/6090006/diff/30005/chrome_frame/chrome_tab.cc#...
chrome_frame/chrome_tab.cc:748: return (result2 == ERROR_SUCCESS) || (result2 ==
ERROR_SUCCESS);
On 2011/01/12 15:01:34, robertshield wrote:
> The || should be an &&: previously both writes/deletes had to succeed for this
> to return true, now only one of them has to succeed.

Done.

[tommi (sloooow) - chröme, 2011-01-14 05:26:47]

I'm completely with Amit, Siggi, Sanjeev, Robert and Greg on this.  The
argument is now not about making life for Windows engineers easier and the
code better but more about not "losing" a debate in an email thread by
coming up with awkward solutions that have the sole purpose of forcing all
methods of a simple, platform specific wrapper class to return bool!?!?

On Thu, Jan 13, 2011 at 11:24 PM, Amit Joshi <amit@chromium.org> wrote:

> Option #1 negates the benefits of a wrapper to a large extent, you can
> notice the difference between the original (where ReadInt64 was rolled out
> inline) and merged. Rolling out ReadValue for wstrings anywhere you need a
> return value is not going to be pretty.
> Option #2: GetLastError() pattern:  Tommi and others have already laid out
> drawbacks.
>
> Now that the disadvantages are clear, lets say we go with Option #1 or #2
> what's the upside?
> Only thing I can think of is that an 'if' expression is less ugly to look
> at.  That's also questionable, because majority of the RegKey users are
> already familiar with the Registry error codes. In fact, it is counter
> intuitive for me that RegKey wrappers return a bool while underlying API
> returns an error code.  A 'SUCCESS' macro can easily fix readability to some
> extent.
> Also, I am dubious about protecting someone with zero windows knowledge and
> intends to use this API without understanding it. I don't think any
> framework can protect against that.
>
> I think benefits of returning error clearly outweigh the drawbacks and yes
> feels like deja vu talking about it...
> wait, did I just say that? :)
>
> On Thu, Jan 13, 2011 at 6:22 PM, <cpu@chromium.org> wrote:
>
>> Note that the method HKEY Handle() is implemented already we we could
>> rewrite
>> amit's example without much trouble to do what he needs. Yes, I am still
>> rooting
>> for my option #1.
>>
>> The disadvantage of the GetLastError() is clear to me. The hope is that
>> because
>> we do this in the wrapper we would not forget, basically the in the code
>> review
>> pattern should be:
>>
>> last_error_ = ::RegXXXX(...);
>>
>> If we follow this in all the methods we should be ok. We wont be using TLS
>> or
>> abusing SetLastError(). So yes, my second vote goes for last_error_.
>>
>> I know we are going in circles now but I think is a good think we get an
>> agreement what the windows wrappers in general are going to hide (or to
>> expose)
>> because the registry wrappers are just but one small class and the outcome
>> of
>> this pretty much opens the door to N^2 more reviews for all the other
>> windows
>> wrappers to start exposing (leaking) the return codes.
>>
>>
>>
>> On 2011/01/13 23:45:42, amit wrote:
>>
>>> On Thu, Jan 13, 2011 at 3:08 PM, Brett Wilson <mailto:
>>> brettw@chromium.org>
>>>
>> wrote:
>>
>>
>>  > On Thu, Jan 13, 2011 at 3:05 PM, Tommi <mailto:tommi@chromium.org>
>>> wrote:
>>> > > A very simple example is RegDeleteValue.  It can fail because the
>>> value
>>> > > doesn't exist, insufficient access, parent key doesn't exist etc.
>>>  You
>>> > > typically do not want to handle each case the same.
>>> >
>>> > Sorry about my ignorance. Can you find some code that does this and
>>> > tell me what should happen in the different cases? It seems from
>>> > looking at this kind of thing in the past is that the two approaches
>>> > are either ignore the error or give up.
>>> >
>>>
>>
>>  For e.g. I just merged with this code.
>>> Original:
>>>
>>
>>    HKEY roots[] = { HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE };
>>>   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;
>>>         *exists = true;
>>>         return true;
>>>       }
>>>     }
>>>   }
>>>
>>
>>  Merged:
>>>
>>
>>    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;
>>>     result = config_key.Open(HKEY_CURRENT_USER, key_name_.c_str(),
>>>                              KEY_QUERY_VALUE);
>>>     if (result == ERROR_SUCCESS) {
>>>       result = config_key.ReadValueInt64(kReadyModeStateValue, 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;
>>>       }
>>>     }
>>>   }
>>>
>>
>>
>>
>>  >
>>> > Brett
>>> >
>>>
>>
>>
>>
>> http://codereview.chromium.org/6090006/
>>
>
>

[brettw, 2011-01-14 07:46:45]

On Thu, Jan 13, 2011 at 3:45 PM, Amit Joshi <amit@chromium.org> wrote:
> On Thu, Jan 13, 2011 at 3:08 PM, Brett Wilson <brettw@chromium.org> wrote:
>>
>> On Thu, Jan 13, 2011 at 3:05 PM, Tommi <tommi@chromium.org> wrote:
>> > A very simple example is RegDeleteValue.  It can fail because the value
>> > doesn't exist, insufficient access, parent key doesn't exist etc.  You
>> > typically do not want to handle each case the same.
>>
>> Sorry about my ignorance. Can you find some code that does this and
>> tell me what should happen in the different cases? It seems from
>> looking at this kind of thing in the past is that the two approaches
>> are either ignore the error or give up.
>
> For e.g. I just merged with this code.
> Original:
>   HKEY roots[] = { HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE };
>   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;
>         *exists = true;
>         return true;
>       }
>     }
>   }
> Merged:
>   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;
>     result = config_key.Open(HKEY_CURRENT_USER, key_name_.c_str(),
>                              KEY_QUERY_VALUE);
>     if (result == ERROR_SUCCESS) {
>       result = config_key.ReadValueInt64(kReadyModeStateValue, 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;
>       }
>     }
>   }

The new code is wrong because you ignore roots[i] inside the loop and
always use LOCAL_MACHINE.

In this particular example, I guess I don't get the point of all the
logging or why we would specifically need to bail out early if there's
an error reading the key from USER but the key also exists in
LOCAL_MACHINE. I would have done the following which looks prettier to
me:

HKEY roots[] = { HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE };
for (int i = 0; i < arraysize(roots); i++) {
    base::win::RegKey config_key(roots[i], key_name_.c_str(),
                              KEY_QUERY_VALUE);
  if (config_key.IsValid() &&
      config_key.ReadValueInt64(kReadyModeStateValue, value)) {
    *exists = true;
    return true;
  }
}
return false;

Brett

[brettw, 2011-01-14 09:33:49]

I think there is a philosophical difference between writing Windows
code in Chrome, and writing Chrome code in Windows. You can write good
quality code in either style, but on our project we should be trying
to write "Chrome-style" code that calls into Windows.

I did a readability review for another project recently. The developer
used HRESULTS for every function in his class. Every function that
called those functions was littered with
  HRESULT hr = Foo();
  if (FAILED(hr))
    return hr;
This is a classic case of writing Windows (or XPCOM) code in Google3.
At some shops, this is desirable and encouraged, but isn't the
approach we usually take here. We discourage Windows types, and
generally log and bail out when something too weird happens. I asked
this developer to consider what errors might actually occur and what
the caller might reasonably be expected to do with the error, and use
assertions and/or bool return values in as many places as possible.
The result was that basically all the HRESULTS and most of the error
checking went away. The code was shorter, more natural to read, and
didn't sacrifice practical robustness.

I feel the same way about this patch. The first version of this change
touched 53 files. All the callsites became longer, uglier, and less
readable, and I think there's agreement that this was the case. I
didn't look at every single one, but I looked at most of them, and I
don't think there was any actual improvement. There have been two
actual examples given of why this needs to change, only one of which
seems to have been actually been changed in this patch.



Since I don't use registry stuff very often and you guys do, I think
it's OK to do this if you think it will increase productivity and code
quality. However, I continue to think that it has a net negative
effect on overall Chrome code quality, as far as what I have seen
goes. I also think some of the arguments in favor of this are to
support generally unnecessary logging, error handling, and
threadsafety requirements, and I hope that this overall approach to
"writing Windows code in Chrome" doesn't spill over too much, as
Carlos was concerned about.

I hope that all of the "awkward" callers are changed over to the new
system soon. It would be very unfortunate for this change to land and
for that to be the end of it and I would suggest it should be reverted
if that's the case.

Chrome style is to use "foo == ERROR_SUCCESS" rather than
"ERROR_SUCCESS == foo", and in a few places I notice you did the
latter. Can you fix these places? I don't think the REG_SUCCEEDED
macro is worth the inherent evil of a macro so I would remove it. Also
please remove the GONG before checkin and fix the bug in the for loop
I mentioned in the previous email (also consider how much of that
logging is really necessary and whether it can be simplified).

Tommi's complaints about my motivations almost made me change my mind
about this.

Brett

[grt (UTC plus 2), 2011-01-14 15:53:43]

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/dele...
File chrome/installer/util/delete_reg_value_work_item.cc (right):

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/dele...
chrome/installer/util/delete_reg_value_work_item.cc:50: if (result ==
ERROR_MORE_DATA) {
Have you verified this?  According to MSDN, you should be testing for
ERROR_SUCCESS here rather than MORE_DATA: "If lpData is NULL, and lpcbData is
non-NULL, the function returns ERROR_SUCCESS and stores the size of the data, in
bytes, in the variable pointed to by lpcbData."

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/dele...
chrome/installer/util/delete_reg_value_work_item.cc:83: result =
key.WriteValue(value_name_.c_str(), previous_value_.c_str(),
If you use std::string where std::vector<uint8> belongs, please use data() to
get at the bytes rather than c_str().

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/dele...
File chrome/installer/util/delete_reg_value_work_item.h (right):

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/dele...
chrome/installer/util/delete_reg_value_work_item.h:59: std::string
previous_value_;
It looks like you're using a std::string as a bucket of bytes.  Why not use an
actual bucket of bytes (std::vector<uint8>)?

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/set_...
File chrome/installer/util/set_reg_value_work_item.cc (right):

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/set_...
chrome/installer/util/set_reg_value_work_item.cc:27: value_.assign(data,
value_data.length() * sizeof(wchar_t));
You must use (value_data.length() + 1) * sizeof(wchar_t) here since you need to
include the terminating NULL in the call to RegSetValueEx.

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/set_...
chrome/installer/util/set_reg_value_work_item.cc:83: if (!overwrite_ && result
!= ERROR_FILE_NOT_FOUND) {
nit: make the order of the tests and the order of the description in the comment
the same.

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/set_...
chrome/installer/util/set_reg_value_work_item.cc:88: if (result ==
ERROR_MORE_DATA) {
Same comment as before: MSDN leads me to believe that ERROR_SUCCESS is the
expected result here.

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/set_...
chrome/installer/util/set_reg_value_work_item.cc:96: result =
key.WriteValue(value_name_.c_str(), value_.c_str(),
If you stick with std::string, use data() rather than c_str().

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/set_...
chrome/installer/util/set_reg_value_work_item.cc:114: status_ = VALUE_ROLL_BACK;
This is wrong.

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/set_...
chrome/installer/util/set_reg_value_work_item.cc:131: result =
key.WriteValue(value_name_.c_str(), previous_value_.c_str(),
If you stick with std::string, use data() rather than c_str().

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/set_...
File chrome/installer/util/set_reg_value_work_item.h (right):

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/set_...
chrome/installer/util/set_reg_value_work_item.h:74: std::string value_;
My objection to using std::string where std::vector<uint8> belongs applies here
as well.

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/set_...
chrome/installer/util/set_reg_value_work_item.h:76: std::string previous_value_;
And here.

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/shel...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:210: read_value == _int_value;
For consistency, this should have parens, no?

http://codereview.chromium.org/6090006/diff/190001/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/6090006/diff/190001/chrome_frame/chrome_tab.cc...
chrome_frame/chrome_tab.cc:335: }
Looks like you're missing "if (ret == ERROR_SUCCESS) result = S_OK;" just before
this line.

http://codereview.chromium.org/6090006/diff/190001/chrome_frame/ready_mode/in...
File chrome_frame/ready_mode/internal/registry_ready_mode_state.cc (right):

http://codereview.chromium.org/6090006/diff/190001/chrome_frame/ready_mode/in...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:195: result =
config_key.Open(HKEY_CURRENT_USER, key_name_.c_str(),
HKEY_CURRENT_USER -> roots[i] as brettw pointed out.

http://codereview.chromium.org/6090006/diff/190001/chrome_frame/utils.cc
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/6090006/diff/190001/chrome_frame/utils.cc#newc...
chrome_frame/utils.cc:260: if (result = ERROR_SUCCESS) {
result == ERROR_SUCCESS
(I still prefer not having the local var in these cases.  I also strongly prefer
ERROR_SUCCESS == result for exactly this reason, but that's a battle I'm not
insane enough to get into now.  :-) )

http://codereview.chromium.org/6090006/diff/190001/net/base/platform_mime_uti...
File net/base/platform_mime_util_win.cc (right):

http://codereview.chromium.org/6090006/diff/190001/net/base/platform_mime_uti...
net/base/platform_mime_util_win.cc:20: LONG ret =
mime_key.Open(HKEY_CLASSES_ROOT, key.c_str(), KEY_READ);
What's the benefit to making this code bigger like this?

http://codereview.chromium.org/6090006/diff/190001/net/base/platform_mime_uti...
net/base/platform_mime_util_win.cc:35: if (ret == ERROR_SUCCESS)
Same comment.

[tommi (sloooow) - chröme, 2011-01-14 16:36:12]

Hey Brett,

I apologize for the tone in my email.  Reading it back it sounds much
more aggressive than what I was trying to get across last night.
I just thought we were making too big a deal out of something trivial.
I wasn't directing the email only at you but regardless, I'm sorry.
Also, just so that there's no misunderstanding, the flaming poo
reference was meant as a jab at myself more than anything,
but I think you know that :)

Tommi

On Fri, Jan 14, 2011 at 4:33 AM, Brett Wilson <brettw@chromium.org> wrote:

> I think there is a philosophical difference between writing Windows
> code in Chrome, and writing Chrome code in Windows. You can write good
> quality code in either style, but on our project we should be trying
> to write "Chrome-style" code that calls into Windows.
>
> I did a readability review for another project recently. The developer
> used HRESULTS for every function in his class. Every function that
> called those functions was littered with
>  HRESULT hr = Foo();
>  if (FAILED(hr))
>    return hr;
> This is a classic case of writing Windows (or XPCOM) code in Google3.
> At some shops, this is desirable and encouraged, but isn't the
> approach we usually take here. We discourage Windows types, and
> generally log and bail out when something too weird happens. I asked
> this developer to consider what errors might actually occur and what
> the caller might reasonably be expected to do with the error, and use
> assertions and/or bool return values in as many places as possible.
> The result was that basically all the HRESULTS and most of the error
> checking went away. The code was shorter, more natural to read, and
> didn't sacrifice practical robustness.
>
> I feel the same way about this patch. The first version of this change
> touched 53 files. All the callsites became longer, uglier, and less
> readable, and I think there's agreement that this was the case. I
> didn't look at every single one, but I looked at most of them, and I
> don't think there was any actual improvement. There have been two
> actual examples given of why this needs to change, only one of which
> seems to have been actually been changed in this patch.
>
>
>
> Since I don't use registry stuff very often and you guys do, I think
> it's OK to do this if you think it will increase productivity and code
> quality. However, I continue to think that it has a net negative
> effect on overall Chrome code quality, as far as what I have seen
> goes. I also think some of the arguments in favor of this are to
> support generally unnecessary logging, error handling, and
> threadsafety requirements, and I hope that this overall approach to
> "writing Windows code in Chrome" doesn't spill over too much, as
> Carlos was concerned about.
>
> I hope that all of the "awkward" callers are changed over to the new
> system soon. It would be very unfortunate for this change to land and
> for that to be the end of it and I would suggest it should be reverted
> if that's the case.
>
> Chrome style is to use "foo == ERROR_SUCCESS" rather than
> "ERROR_SUCCESS == foo", and in a few places I notice you did the
> latter. Can you fix these places? I don't think the REG_SUCCEEDED
> macro is worth the inherent evil of a macro so I would remove it. Also
> please remove the GONG before checkin and fix the bug in the for loop
> I mentioned in the previous email (also consider how much of that
> logging is really necessary and whether it can be simplified).
>
> Tommi's complaints about my motivations almost made me change my mind
> about this.
>
> Brett
>

[cpu_(ooo_6.6-7.5), 2011-01-14 21:34:07]

Ok, if we don't buy the making it easy to use for non-windows people why we do
we have the CatchBoolChecks class?

I'll make you a deal, I'll shut up if the function returns a plain LONG.


On 2011/01/14 16:36:12, tommi wrote:
> Hey Brett,
> 
> I apologize for the tone in my email.  Reading it back it sounds much
> more aggressive than what I was trying to get across last night.
> I just thought we were making too big a deal out of something trivial.
> I wasn't directing the email only at you but regardless, I'm sorry.
> Also, just so that there's no misunderstanding, the flaming poo
> reference was meant as a jab at myself more than anything,
> but I think you know that :)
> 
> Tommi
> 
> On Fri, Jan 14, 2011 at 4:33 AM, Brett Wilson <mailto:brettw@chromium.org>
wrote:
> 
> > I think there is a philosophical difference between writing Windows
> > code in Chrome, and writing Chrome code in Windows. You can write good
> > quality code in either style, but on our project we should be trying
> > to write "Chrome-style" code that calls into Windows.
> >
> > I did a readability review for another project recently. The developer
> > used HRESULTS for every function in his class. Every function that
> > called those functions was littered with
> >  HRESULT hr = Foo();
> >  if (FAILED(hr))
> >    return hr;
> > This is a classic case of writing Windows (or XPCOM) code in Google3.
> > At some shops, this is desirable and encouraged, but isn't the
> > approach we usually take here. We discourage Windows types, and
> > generally log and bail out when something too weird happens. I asked
> > this developer to consider what errors might actually occur and what
> > the caller might reasonably be expected to do with the error, and use
> > assertions and/or bool return values in as many places as possible.
> > The result was that basically all the HRESULTS and most of the error
> > checking went away. The code was shorter, more natural to read, and
> > didn't sacrifice practical robustness.
> >
> > I feel the same way about this patch. The first version of this change
> > touched 53 files. All the callsites became longer, uglier, and less
> > readable, and I think there's agreement that this was the case. I
> > didn't look at every single one, but I looked at most of them, and I
> > don't think there was any actual improvement. There have been two
> > actual examples given of why this needs to change, only one of which
> > seems to have been actually been changed in this patch.
> >
> >
> >
> > Since I don't use registry stuff very often and you guys do, I think
> > it's OK to do this if you think it will increase productivity and code
> > quality. However, I continue to think that it has a net negative
> > effect on overall Chrome code quality, as far as what I have seen
> > goes. I also think some of the arguments in favor of this are to
> > support generally unnecessary logging, error handling, and
> > threadsafety requirements, and I hope that this overall approach to
> > "writing Windows code in Chrome" doesn't spill over too much, as
> > Carlos was concerned about.
> >
> > I hope that all of the "awkward" callers are changed over to the new
> > system soon. It would be very unfortunate for this change to land and
> > for that to be the end of it and I would suggest it should be reverted
> > if that's the case.
> >
> > Chrome style is to use "foo == ERROR_SUCCESS" rather than
> > "ERROR_SUCCESS == foo", and in a few places I notice you did the
> > latter. Can you fix these places? I don't think the REG_SUCCEEDED
> > macro is worth the inherent evil of a macro so I would remove it. Also
> > please remove the GONG before checkin and fix the bug in the for loop
> > I mentioned in the previous email (also consider how much of that
> > logging is really necessary and whether it can be simplified).
> >
> > Tommi's complaints about my motivations almost made me change my mind
> > about this.
> >
> > Brett
> >

[amit, 2011-01-14 22:38:15]

On Fri, Jan 14, 2011 at 1:34 PM, <cpu@chromium.org> wrote:

> Ok, if we don't buy the making it easy to use for non-windows people why we
> do
> we have the CatchBoolChecks class?
>
> I'll make you a deal, I'll shut up if the function returns a plain LONG.


That was the intention right from start.  CatchBoolChecks is documented as a
temporary hack top help me catch new if (!regkey...) checks after each
merge.  I was contemplating whether to remove it before checkin or leave it
there for a couple of days to help others who had changes in flight catch
such errors.


>
>
> On 2011/01/14 16:36:12, tommi wrote:
>
>> Hey Brett,
>>
>
>  I apologize for the tone in my email.  Reading it back it sounds much
>> more aggressive than what I was trying to get across last night.
>> I just thought we were making too big a deal out of something trivial.
>> I wasn't directing the email only at you but regardless, I'm sorry.
>> Also, just so that there's no misunderstanding, the flaming poo
>> reference was meant as a jab at myself more than anything,
>> but I think you know that :)
>>
>
>  Tommi
>>
>
>  On Fri, Jan 14, 2011 at 4:33 AM, Brett Wilson <mailto:brettw@chromium.org
>> >
>>
> wrote:
>
>  > I think there is a philosophical difference between writing Windows
>> > code in Chrome, and writing Chrome code in Windows. You can write good
>> > quality code in either style, but on our project we should be trying
>> > to write "Chrome-style" code that calls into Windows.
>> >
>> > I did a readability review for another project recently. The developer
>> > used HRESULTS for every function in his class. Every function that
>> > called those functions was littered with
>> >  HRESULT hr = Foo();
>> >  if (FAILED(hr))
>> >    return hr;
>> > This is a classic case of writing Windows (or XPCOM) code in Google3.
>> > At some shops, this is desirable and encouraged, but isn't the
>> > approach we usually take here. We discourage Windows types, and
>> > generally log and bail out when something too weird happens. I asked
>> > this developer to consider what errors might actually occur and what
>> > the caller might reasonably be expected to do with the error, and use
>> > assertions and/or bool return values in as many places as possible.
>> > The result was that basically all the HRESULTS and most of the error
>> > checking went away. The code was shorter, more natural to read, and
>> > didn't sacrifice practical robustness.
>> >
>> > I feel the same way about this patch. The first version of this change
>> > touched 53 files. All the callsites became longer, uglier, and less
>> > readable, and I think there's agreement that this was the case. I
>> > didn't look at every single one, but I looked at most of them, and I
>> > don't think there was any actual improvement. There have been two
>> > actual examples given of why this needs to change, only one of which
>> > seems to have been actually been changed in this patch.
>> >
>> >
>> >
>> > Since I don't use registry stuff very often and you guys do, I think
>> > it's OK to do this if you think it will increase productivity and code
>> > quality. However, I continue to think that it has a net negative
>> > effect on overall Chrome code quality, as far as what I have seen
>> > goes. I also think some of the arguments in favor of this are to
>> > support generally unnecessary logging, error handling, and
>> > threadsafety requirements, and I hope that this overall approach to
>> > "writing Windows code in Chrome" doesn't spill over too much, as
>> > Carlos was concerned about.
>> >
>> > I hope that all of the "awkward" callers are changed over to the new
>> > system soon. It would be very unfortunate for this change to land and
>> > for that to be the end of it and I would suggest it should be reverted
>> > if that's the case.
>> >
>> > Chrome style is to use "foo == ERROR_SUCCESS" rather than
>> > "ERROR_SUCCESS == foo", and in a few places I notice you did the
>> > latter. Can you fix these places? I don't think the REG_SUCCEEDED
>> > macro is worth the inherent evil of a macro so I would remove it. Also
>> > please remove the GONG before checkin and fix the bug in the for loop
>> > I mentioned in the previous email (also consider how much of that
>> > logging is really necessary and whether it can be simplified).
>> >
>> > Tommi's complaints about my motivations almost made me change my mind
>> > about this.
>> >
>> > Brett
>> >
>>
>
>
>
> http://codereview.chromium.org/6090006/
>

[cpu_(ooo_6.6-7.5), 2011-01-14 22:45:00]

lgtm with the removal now. Somebody could have a CL written for 5 weeks so I
don't see much advantage of keeping it.

I also think brettw is done with it so please proceed with checkin and sorry for
the churn.


On 2011/01/14 22:38:15, amit wrote:
> On Fri, Jan 14, 2011 at 1:34 PM, <mailto:cpu@chromium.org> wrote:
> 
> > Ok, if we don't buy the making it easy to use for non-windows people why we
> > do
> > we have the CatchBoolChecks class?
> >
> > I'll make you a deal, I'll shut up if the function returns a plain LONG.
> 
> 
> That was the intention right from start.  CatchBoolChecks is documented as a
> temporary hack top help me catch new if (!regkey...) checks after each
> merge.  I was contemplating whether to remove it before checkin or leave it
> there for a couple of days to help others who had changes in flight catch
> such errors.
> 
> 
> >
> >
> > On 2011/01/14 16:36:12, tommi wrote:
> >
> >> Hey Brett,
> >>
> >
> >  I apologize for the tone in my email.  Reading it back it sounds much
> >> more aggressive than what I was trying to get across last night.
> >> I just thought we were making too big a deal out of something trivial.
> >> I wasn't directing the email only at you but regardless, I'm sorry.
> >> Also, just so that there's no misunderstanding, the flaming poo
> >> reference was meant as a jab at myself more than anything,
> >> but I think you know that :)
> >>
> >
> >  Tommi
> >>
> >
> >  On Fri, Jan 14, 2011 at 4:33 AM, Brett Wilson <mailto:brettw@chromium.org
> >> >
> >>
> > wrote:
> >
> >  > I think there is a philosophical difference between writing Windows
> >> > code in Chrome, and writing Chrome code in Windows. You can write good
> >> > quality code in either style, but on our project we should be trying
> >> > to write "Chrome-style" code that calls into Windows.
> >> >
> >> > I did a readability review for another project recently. The developer
> >> > used HRESULTS for every function in his class. Every function that
> >> > called those functions was littered with
> >> >  HRESULT hr = Foo();
> >> >  if (FAILED(hr))
> >> >    return hr;
> >> > This is a classic case of writing Windows (or XPCOM) code in Google3.
> >> > At some shops, this is desirable and encouraged, but isn't the
> >> > approach we usually take here. We discourage Windows types, and
> >> > generally log and bail out when something too weird happens. I asked
> >> > this developer to consider what errors might actually occur and what
> >> > the caller might reasonably be expected to do with the error, and use
> >> > assertions and/or bool return values in as many places as possible.
> >> > The result was that basically all the HRESULTS and most of the error
> >> > checking went away. The code was shorter, more natural to read, and
> >> > didn't sacrifice practical robustness.
> >> >
> >> > I feel the same way about this patch. The first version of this change
> >> > touched 53 files. All the callsites became longer, uglier, and less
> >> > readable, and I think there's agreement that this was the case. I
> >> > didn't look at every single one, but I looked at most of them, and I
> >> > don't think there was any actual improvement. There have been two
> >> > actual examples given of why this needs to change, only one of which
> >> > seems to have been actually been changed in this patch.
> >> >
> >> >
> >> >
> >> > Since I don't use registry stuff very often and you guys do, I think
> >> > it's OK to do this if you think it will increase productivity and code
> >> > quality. However, I continue to think that it has a net negative
> >> > effect on overall Chrome code quality, as far as what I have seen
> >> > goes. I also think some of the arguments in favor of this are to
> >> > support generally unnecessary logging, error handling, and
> >> > threadsafety requirements, and I hope that this overall approach to
> >> > "writing Windows code in Chrome" doesn't spill over too much, as
> >> > Carlos was concerned about.
> >> >
> >> > I hope that all of the "awkward" callers are changed over to the new
> >> > system soon. It would be very unfortunate for this change to land and
> >> > for that to be the end of it and I would suggest it should be reverted
> >> > if that's the case.
> >> >
> >> > Chrome style is to use "foo == ERROR_SUCCESS" rather than
> >> > "ERROR_SUCCESS == foo", and in a few places I notice you did the
> >> > latter. Can you fix these places? I don't think the REG_SUCCEEDED
> >> > macro is worth the inherent evil of a macro so I would remove it. Also
> >> > please remove the GONG before checkin and fix the bug in the for loop
> >> > I mentioned in the previous email (also consider how much of that
> >> > logging is really necessary and whether it can be simplified).
> >> >
> >> > Tommi's complaints about my motivations almost made me change my mind
> >> > about this.
> >> >
> >> > Brett
> >> >
> >>
> >
> >
> >
> > http://codereview.chromium.org/6090006/
> >

[amit, 2011-01-15 01:28:11]

New patch up. Also removed majority of temporary vars (except where being used
in LOGs) as per popular request.

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/dele...
File chrome/installer/util/delete_reg_value_work_item.cc (right):

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/dele...
chrome/installer/util/delete_reg_value_work_item.cc:50: if (result ==
ERROR_MORE_DATA) {
On 2011/01/14 15:53:43, grt wrote:
> Have you verified this?  According to MSDN, you should be testing for
> ERROR_SUCCESS here rather than MORE_DATA: "If lpData is NULL, and lpcbData is
> non-NULL, the function returns ERROR_SUCCESS and stores the size of the data,
in
> bytes, in the variable pointed to by lpcbData."

Done.

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/dele...
File chrome/installer/util/delete_reg_value_work_item.h (right):

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/dele...
chrome/installer/util/delete_reg_value_work_item.h:59: std::string
previous_value_;
On 2011/01/14 15:53:43, grt wrote:
> It looks like you're using a std::string as a bucket of bytes.  Why not use an
> actual bucket of bytes (std::vector<uint8>)?

Yep, back to vector :)
I started with std::vector then thought using WriteInto would be cool with
std::string so switched to it. Now discovered that WriteInto is not going to
work for non-strings.

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/set_...
File chrome/installer/util/set_reg_value_work_item.cc (right):

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/set_...
chrome/installer/util/set_reg_value_work_item.cc:27: value_.assign(data,
value_data.length() * sizeof(wchar_t));
On 2011/01/14 15:53:43, grt wrote:
> You must use (value_data.length() + 1) * sizeof(wchar_t) here since you need
to
> include the terminating NULL in the call to RegSetValueEx.

Done.

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/set_...
chrome/installer/util/set_reg_value_work_item.cc:83: if (!overwrite_ && result
!= ERROR_FILE_NOT_FOUND) {
On 2011/01/14 15:53:43, grt wrote:
> nit: make the order of the tests and the order of the description in the
comment
> the same.

Done.

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/set_...
chrome/installer/util/set_reg_value_work_item.cc:88: if (result ==
ERROR_MORE_DATA) {
On 2011/01/14 15:53:43, grt wrote:
> Same comment as before: MSDN leads me to believe that ERROR_SUCCESS is the
> expected result here.

Done.

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/set_...
chrome/installer/util/set_reg_value_work_item.cc:96: result =
key.WriteValue(value_name_.c_str(), value_.c_str(),
On 2011/01/14 15:53:43, grt wrote:
> If you stick with std::string, use data() rather than c_str().

Done.

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/set_...
chrome/installer/util/set_reg_value_work_item.cc:114: status_ = VALUE_ROLL_BACK;
On 2011/01/14 15:53:43, grt wrote:
> This is wrong.
Good catch, removed.

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/shel...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/6090006/diff/190001/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:210: read_value == _int_value;
On 2011/01/14 15:53:43, grt wrote:
> For consistency, this should have parens, no?

Done.

http://codereview.chromium.org/6090006/diff/190001/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/6090006/diff/190001/chrome_frame/chrome_tab.cc...
chrome_frame/chrome_tab.cc:335: }
On 2011/01/14 15:53:43, grt wrote:
> Looks like you're missing "if (ret == ERROR_SUCCESS) result = S_OK;" just
before
> this line.
Next HRESULT_FROM_WIN32 should translate ERROR_SUCCESS to S_OK.

http://codereview.chromium.org/6090006/diff/190001/chrome_frame/ready_mode/in...
File chrome_frame/ready_mode/internal/registry_ready_mode_state.cc (right):

http://codereview.chromium.org/6090006/diff/190001/chrome_frame/ready_mode/in...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:195: result =
config_key.Open(HKEY_CURRENT_USER, key_name_.c_str(),
On 2011/01/14 15:53:43, grt wrote:
> HKEY_CURRENT_USER -> roots[i] as brettw pointed out.

Done.

http://codereview.chromium.org/6090006/diff/190001/chrome_frame/utils.cc
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/6090006/diff/190001/chrome_frame/utils.cc#newc...
chrome_frame/utils.cc:260: if (result = ERROR_SUCCESS) {
On 2011/01/14 15:53:43, grt wrote:
> result == ERROR_SUCCESS
> (I still prefer not having the local var in these cases.  I also strongly
prefer
> ERROR_SUCCESS == result for exactly this reason, but that's a battle I'm not
> insane enough to get into now.  :-) )

Done.

http://codereview.chromium.org/6090006/diff/190001/net/base/platform_mime_uti...
File net/base/platform_mime_util_win.cc (right):

http://codereview.chromium.org/6090006/diff/190001/net/base/platform_mime_uti...
net/base/platform_mime_util_win.cc:20: LONG ret =
mime_key.Open(HKEY_CLASSES_ROOT, key.c_str(), KEY_READ);
On 2011/01/14 15:53:43, grt wrote:
> What's the benefit to making this code bigger like this?
reverted

http://codereview.chromium.org/6090006/diff/190001/net/base/platform_mime_uti...
net/base/platform_mime_util_win.cc:35: if (ret == ERROR_SUCCESS)
On 2011/01/14 15:53:43, grt wrote:
> Same comment.

Done.

[grt (UTC plus 2), 2011-01-16 04:19:48]

LGTM with some nits mostly about line wrapping and indentation.  Feel free to
change or not based on your preference (I didn't survey the surrounding code to
see if it's more readable as-is).

http://codereview.chromium.org/6090006/diff/190001/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/6090006/diff/190001/chrome_frame/chrome_tab.cc...
chrome_frame/chrome_tab.cc:335: }
On 2011/01/15 01:28:11, amit wrote:
> On 2011/01/14 15:53:43, grt wrote:
> > Looks like you're missing "if (ret == ERROR_SUCCESS) result = S_OK;" just
> before
> > this line.
> Next HRESULT_FROM_WIN32 should translate ERROR_SUCCESS to S_OK.

Oh yeah.  Sweet.

http://codereview.chromium.org/6090006/diff/240002/base/win/win_util.cc
File base/win/win_util.cc (right):

http://codereview.chromium.org/6090006/diff/240002/base/win/win_util.cc#newco...
base/win/win_util.cc:132: == ERROR_SUCCESS);
nit: Chrome style guide says to wrap after the operator, not before.  also,
align ERROR_SUCCESS with the open paren, no?

http://codereview.chromium.org/6090006/diff/240002/ceee/ie/common/ceee_module...
File ceee/ie/common/ceee_module_util.cc (right):

http://codereview.chromium.org/6090006/diff/240002/ceee/ie/common/ceee_module...
ceee/ie/common/ceee_module_util.cc:111: break;
Add braces around this line since the if spans multiple lines.

http://codereview.chromium.org/6090006/diff/240002/ceee/ie/common/ceee_util.cc
File ceee/ie/common/ceee_util.cc (right):

http://codereview.chromium.org/6090006/diff/240002/ceee/ie/common/ceee_util.c...
ceee/ie/common/ceee_util.cc:27: == ERROR_SUCCESS);
Wrapping and indentation

http://codereview.chromium.org/6090006/diff/240002/ceee/ie/plugin/toolband/to...
File ceee/ie/plugin/toolband/toolband_proxy.cc (right):

http://codereview.chromium.org/6090006/diff/240002/ceee/ie/plugin/toolband/to...
ceee/ie/plugin/toolband/toolband_proxy.cc:55: == ERROR_SUCCESS) {
Wrapping

http://codereview.chromium.org/6090006/diff/240002/ceee/installer_dll/install...
File ceee/installer_dll/installer_helper.cc (right):

http://codereview.chromium.org/6090006/diff/240002/ceee/installer_dll/install...
ceee/installer_dll/installer_helper.cc:262: != ERROR_SUCCESS) {
Wrapping

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/chan...
File chrome/installer/util/channel_info.cc (right):

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/chan...
chrome/installer/util/channel_info.cc:124: == ERROR_SUCCESS);
Wrapping and indentation.

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/dele...
File chrome/installer/util/delete_after_reboot_helper.cc (right):

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/dele...
chrome/installer/util/delete_after_reboot_helper.cc:383: == ERROR_SUCCESS);
Wrapping and indentation.

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/dele...
chrome/installer/util/delete_after_reboot_helper.cc:391: buffer.size(),
REG_MULTI_SZ) == ERROR_SUCCESS);
Should this be indented 4 spaces from the open paren above?

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/goog...
File chrome/installer/util/google_update_settings.cc (right):

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:99: != ERROR_SUCCESS) {
Wrapping

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:117: == ERROR_SUCCESS);
Wrapping and indentation

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/goog...
File chrome/installer/util/google_update_settings_unittest.cc (right):

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/goog...
chrome/installer/util/google_update_settings_unittest.cc:171: == ERROR_SUCCESS)
{
Wrapping

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/goog...
chrome/installer/util/google_update_settings_unittest.cc:376: != ERROR_SUCCESS)
{
Wrapping

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/inst...
File chrome/installer/util/installation_state.cc (right):

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/inst...
chrome/installer/util/installation_state.cc:26: == ERROR_SUCCESS) {
Wrapping

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/inst...
chrome/installer/util/installation_state.cc:32: != ERROR_SUCCESS ||
!channel_.Initialize(key)) {
Wrapping (plus extra parens around the first part?)

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/pack...
File chrome/installer/util/package_unittest.cc (right):

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/pack...
chrome/installer/util/package_unittest.cc:148: == ERROR_SUCCESS);
Wrapping and indentation

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/set_...
File chrome/installer/util/set_reg_value_work_item.cc (right):

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/set_...
chrome/installer/util/set_reg_value_work_item.cc:87: return true;
Nice catch!

http://codereview.chromium.org/6090006/diff/240002/chrome/test/mini_installer...
File chrome/test/mini_installer_test/chrome_mini_installer.cc (right):

http://codereview.chromium.org/6090006/diff/240002/chrome/test/mini_installer...
chrome/test/mini_installer_test/chrome_mini_installer.cc:450: == ERROR_SUCCESS)
Wrapping

http://codereview.chromium.org/6090006/diff/240002/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/6090006/diff/240002/chrome_frame/chrome_tab.cc...
chrome_frame/chrome_tab.cc:448: LRESULT ret = ::RegEnumValue(ua_key.Handle(),
value_index, value_name,
Is RegistryValueIterator not suitable for use here?

http://codereview.chromium.org/6090006/diff/240002/chrome_frame/utils.cc
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/6090006/diff/240002/chrome_frame/utils.cc#newc...
chrome_frame/utils.cc:237: == ERROR_SUCCESS);
Wrapping and indentation

http://codereview.chromium.org/6090006/diff/240002/chrome_frame/utils.cc#newc...
chrome_frame/utils.cc:260: == ERROR_SUCCESS) {
Wrapping

http://codereview.chromium.org/6090006/diff/240002/chrome_frame/utils.cc#newc...
chrome_frame/utils.cc:758: return RENDERER_TYPE_UNDETERMINED;
Add braces around this line since the if spans multiple lines.

http://codereview.chromium.org/6090006/diff/240002/webkit/plugins/npapi/plugi...
File webkit/plugins/npapi/plugin_list_win.cc (right):

http://codereview.chromium.org/6090006/diff/240002/webkit/plugins/npapi/plugi...
webkit/plugins/npapi/plugin_list_win.cc:194: != ERROR_SUCCESS)
Wrapping

http://codereview.chromium.org/6090006/diff/240002/webkit/plugins/npapi/plugi...
webkit/plugins/npapi/plugin_list_win.cc:204: == ERROR_SUCCESS) {
Wrapping

[amit, 2011-01-16 07:54:27]

Thanks for your patience and comments for far. New (and hopefully final) patch
up if you want to see what will land.

http://codereview.chromium.org/6090006/diff/240002/base/win/win_util.cc
File base/win/win_util.cc (right):

http://codereview.chromium.org/6090006/diff/240002/base/win/win_util.cc#newco...
base/win/win_util.cc:132: == ERROR_SUCCESS);
On 2011/01/16 04:19:48, grt wrote:
> nit: Chrome style guide says to wrap after the operator, not before.  also,
> align ERROR_SUCCESS with the open paren, no?

Done.

http://codereview.chromium.org/6090006/diff/240002/ceee/ie/common/ceee_module...
File ceee/ie/common/ceee_module_util.cc (right):

http://codereview.chromium.org/6090006/diff/240002/ceee/ie/common/ceee_module...
ceee/ie/common/ceee_module_util.cc:111: break;
On 2011/01/16 04:19:48, grt wrote:
> Add braces around this line since the if spans multiple lines.

Done.

http://codereview.chromium.org/6090006/diff/240002/ceee/ie/common/ceee_util.cc
File ceee/ie/common/ceee_util.cc (right):

http://codereview.chromium.org/6090006/diff/240002/ceee/ie/common/ceee_util.c...
ceee/ie/common/ceee_util.cc:27: == ERROR_SUCCESS);
On 2011/01/16 04:19:48, grt wrote:
> Wrapping and indentation

Done.

http://codereview.chromium.org/6090006/diff/240002/ceee/ie/plugin/toolband/to...
File ceee/ie/plugin/toolband/toolband_proxy.cc (right):

http://codereview.chromium.org/6090006/diff/240002/ceee/ie/plugin/toolband/to...
ceee/ie/plugin/toolband/toolband_proxy.cc:55: == ERROR_SUCCESS) {
On 2011/01/16 04:19:48, grt wrote:
> Wrapping

Done.

http://codereview.chromium.org/6090006/diff/240002/ceee/installer_dll/install...
File ceee/installer_dll/installer_helper.cc (right):

http://codereview.chromium.org/6090006/diff/240002/ceee/installer_dll/install...
ceee/installer_dll/installer_helper.cc:262: != ERROR_SUCCESS) {
On 2011/01/16 04:19:48, grt wrote:
> Wrapping

Done.

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/chan...
File chrome/installer/util/channel_info.cc (right):

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/chan...
chrome/installer/util/channel_info.cc:124: == ERROR_SUCCESS);
On 2011/01/16 04:19:48, grt wrote:
> Wrapping and indentation.

Done.

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/dele...
File chrome/installer/util/delete_after_reboot_helper.cc (right):

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/dele...
chrome/installer/util/delete_after_reboot_helper.cc:383: == ERROR_SUCCESS);
On 2011/01/16 04:19:48, grt wrote:
> Wrapping and indentation.

Done.

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/dele...
chrome/installer/util/delete_after_reboot_helper.cc:391: buffer.size(),
REG_MULTI_SZ) == ERROR_SUCCESS);
On 2011/01/16 04:19:48, grt wrote:
> Should this be indented 4 spaces from the open paren above?
If it fits in one line. It does not and it will make ERROR_SUCCESS dangle on the
next line 6 spaces from the left :)

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/goog...
File chrome/installer/util/google_update_settings.cc (right):

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:99: != ERROR_SUCCESS) {
On 2011/01/16 04:19:48, grt wrote:
> Wrapping

Done.

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/goog...
chrome/installer/util/google_update_settings.cc:117: == ERROR_SUCCESS);
On 2011/01/16 04:19:48, grt wrote:
> Wrapping and indentation

Done.

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/goog...
File chrome/installer/util/google_update_settings_unittest.cc (right):

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/goog...
chrome/installer/util/google_update_settings_unittest.cc:171: == ERROR_SUCCESS)
{
On 2011/01/16 04:19:48, grt wrote:
> Wrapping

Done.

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/goog...
chrome/installer/util/google_update_settings_unittest.cc:376: != ERROR_SUCCESS)
{
On 2011/01/16 04:19:48, grt wrote:
> Wrapping

Done.

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/inst...
File chrome/installer/util/installation_state.cc (right):

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/inst...
chrome/installer/util/installation_state.cc:26: == ERROR_SUCCESS) {
On 2011/01/16 04:19:48, grt wrote:
> Wrapping

Done.

http://codereview.chromium.org/6090006/diff/240002/chrome/installer/util/inst...
chrome/installer/util/installation_state.cc:32: != ERROR_SUCCESS ||
!channel_.Initialize(key)) {
On 2011/01/16 04:19:48, grt wrote:
> Wrapping (plus extra parens around the first part?)

Done.

http://codereview.chromium.org/6090006/diff/240002/chrome/test/mini_installer...
File chrome/test/mini_installer_test/chrome_mini_installer.cc (right):

http://codereview.chromium.org/6090006/diff/240002/chrome/test/mini_installer...
chrome/test/mini_installer_test/chrome_mini_installer.cc:450: == ERROR_SUCCESS)
On 2011/01/16 04:19:48, grt wrote:
> Wrapping

Done.

http://codereview.chromium.org/6090006/diff/240002/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/6090006/diff/240002/chrome_frame/chrome_tab.cc...
chrome_frame/chrome_tab.cc:448: LRESULT ret = ::RegEnumValue(ua_key.Handle(),
value_index, value_name,
On 2011/01/16 04:19:48, grt wrote:
> Is RegistryValueIterator not suitable for use here?

Unfortunately no since it will involve opening the RegKey again and another
iteration to find matches.

http://codereview.chromium.org/6090006/diff/240002/chrome_frame/utils.cc
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/6090006/diff/240002/chrome_frame/utils.cc#newc...
chrome_frame/utils.cc:237: == ERROR_SUCCESS);
On 2011/01/16 04:19:48, grt wrote:
> Wrapping and indentation

Done.

http://codereview.chromium.org/6090006/diff/240002/chrome_frame/utils.cc#newc...
chrome_frame/utils.cc:260: == ERROR_SUCCESS) {
On 2011/01/16 04:19:48, grt wrote:
> Wrapping

Done.

http://codereview.chromium.org/6090006/diff/240002/chrome_frame/utils.cc#newc...
chrome_frame/utils.cc:758: return RENDERER_TYPE_UNDETERMINED;
On 2011/01/16 04:19:48, grt wrote:
> Add braces around this line since the if spans multiple lines.

Done.

http://codereview.chromium.org/6090006/diff/240002/webkit/plugins/npapi/plugi...
File webkit/plugins/npapi/plugin_list_win.cc (right):

http://codereview.chromium.org/6090006/diff/240002/webkit/plugins/npapi/plugi...
webkit/plugins/npapi/plugin_list_win.cc:194: != ERROR_SUCCESS)
On 2011/01/16 04:19:48, grt wrote:
> Wrapping

Done.