C++ Readability for grt.

C++ Readability for grt.

Added CopyRegKeyWorkItem in support of IE low rights policy fixes.  In so doing, I pulled RegistryKeyBackup out of DeleteRegKeyWorkItem (which I wrote back in http://codereview.chromium.org/6598065) so it could be shared between the two work items.  registry_test_data is, as the name implies, some infrastructure shared among a few unit tests.

BUG=none
TEST=none

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

[Yash, 2011-10-03 02:17:37]

Hi Greg, here's my first round of comments.
Your code looks really good! If you have any questions or clarifications, feel
free to check the style guide
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml) or just ask me.

Yash

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/copy_r...
File chrome/installer/util/copy_reg_key_work_item.cc (right):

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/copy_r...
chrome/installer/util/copy_reg_key_work_item.cc:4: 
Please include a brief file comment, describing what you implement here.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#File_Comments

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/copy_r...
chrome/installer/util/copy_reg_key_work_item.cc:70: // -- FALL THROUGH TO
SUCCESS CASE --
No need to have this comment in all caps

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/copy_r...
File chrome/installer/util/copy_reg_key_work_item.h (right):

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/copy_r...
chrome/installer/util/copy_reg_key_work_item.h:7: #pragma once
Why do you use both the ifdef (required by the style guide) and the #pragma once
declarations here?
Only one should be required. Please remove the #pragma.

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/copy_r...
chrome/installer/util/copy_reg_key_work_item.h:24: virtual bool Do() OVERRIDE;
What's the OVERRIDE definition here for?

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/copy_r...
chrome/installer/util/copy_reg_key_work_item.h:28: friend class WorkItem;
Please comment on why WorkItem is a friend class.

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/regist...
File chrome/installer/util/registry_key_backup.cc (right):

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/regist...
chrome/installer/util/registry_key_backup.cc:25: bool Initialize(const RegKey&
key);
Please add comments for these functions and other members in this class.

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/regist...
File chrome/installer/util/registry_key_backup.h (right):

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/regist...
chrome/installer/util/registry_key_backup.h:52: scoped_ptr<KeyData> key_data_;
Please add a quick comment for this member.

[grt (UTC plus 2), 2011-10-03 15:35:26]

Thanks Yash, PTAL.  I think I've addressed all of your comments.  Please note
that I made an interim change to copy_reg_key_work_item.{cc,h} a little over a
week ago (see
http://codereview.chromium.org/7976045/diff/25001/chrome/installer/util/copy_...
if you're curious).  It added one parameter to the ctor, one new data member,
and changed the logic of CopyRegKeyWorkItem::Do and CopyRegKeyWorkItem::Rollback
slightly.

Greg

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/copy_r...
File chrome/installer/util/copy_reg_key_work_item.cc (right):

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/copy_r...
chrome/installer/util/copy_reg_key_work_item.cc:4: 
On 2011/10/03 02:17:37, Yash wrote:
> Please include a brief file comment, describing what you implement here.
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#File_Comments

Done.

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/copy_r...
chrome/installer/util/copy_reg_key_work_item.cc:70: // -- FALL THROUGH TO
SUCCESS CASE --
On 2011/10/03 02:17:37, Yash wrote:
> No need to have this comment in all caps

Done.

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/copy_r...
File chrome/installer/util/copy_reg_key_work_item.h (right):

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/copy_r...
chrome/installer/util/copy_reg_key_work_item.h:7: #pragma once
On 2011/10/03 02:17:37, Yash wrote:
> Why do you use both the ifdef (required by the style guide) and the #pragma
once
> declarations here?
> Only one should be required. Please remove the #pragma.

This is one of the quirks of the Chromium coding style: "When writing header
(.h) files, use both standard include guards and #pragma once."
(http://www.chromium.org/developers/coding-style#TOC-File-headers)

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/copy_r...
chrome/installer/util/copy_reg_key_work_item.h:24: virtual bool Do() OVERRIDE;
On 2011/10/03 02:17:37, Yash wrote:
> What's the OVERRIDE definition here for?

It's a Chromium macro that expands to a compiler-specific annotation that will
trigger a compile error if this suddenly declares a new function rather than an
override of a base class's virtual function (e.g., if the base's Do() is renamed
or changed in some other way). 
(http://www.chromium.org/developers/coding-style#TOC-Code-formatting)

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/copy_r...
chrome/installer/util/copy_reg_key_work_item.h:28: friend class WorkItem;
On 2011/10/03 02:17:37, Yash wrote:
> Please comment on why WorkItem is a friend class.

Done.

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/regist...
File chrome/installer/util/registry_key_backup.cc (right):

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/regist...
chrome/installer/util/registry_key_backup.cc:25: bool Initialize(const RegKey&
key);
On 2011/10/03 02:17:37, Yash wrote:
> Please add comments for these functions and other members in this class.

Done.

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/regist...
File chrome/installer/util/registry_key_backup.h (right):

http://codereview.chromium.org/7946003/diff/2001/chrome/installer/util/regist...
chrome/installer/util/registry_key_backup.h:52: scoped_ptr<KeyData> key_data_;
On 2011/10/03 02:17:37, Yash wrote:
> Please add a quick comment for this member.

Done.

[grt (UTC plus 2), 2011-10-07 15:30:18]

ping

[Yash, 2011-10-09 20:26:46]

LGTM

Thanks for fixing up your code. Once you submit, I'll close the bug and you
should have readability shortly afterwards.

[commit-bot: I haz the power, 2011-10-10 00:18:10]

No LGTM from valid reviewers yet.

[grt (UTC plus 2), 2011-10-10 01:00:04]

On 2011/10/09 20:26:46, Yash wrote:
> LGTM
> 
> Thanks for fixing up your code. Once you submit, I'll close the bug and you
> should have readability shortly afterwards.

Thanks for the review, Yash.  I've landed the change at http://crrev.com/104688.

Cheers,

Greg