Added CopyRegKeyWorkItem in support of IE low rights policy fixes.

Added CopyRegKeyWorkItem in support of IE low rights policy fixes.

A subsequent CL will contain the rest of the fixes for the bug.

BUG=95810
TEST=installer_util_unittests.exe

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

[erikwright (departed), 2011-09-15 18:38:57]

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
File chrome/installer/util/copy_reg_key_work_item.cc (right):

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item.cc:18: }
}  // namespace

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item.cc:42: LONG result =
dest_key.Open(predefined_root_, dest_key_path_.c_str(),
Any chance you can make this a little DRYer?

http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/installer/util...

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item.cc:59: LONG result =
SHDeleteKey(predefined_root_, dest_key_path_.c_str());
Should you not, immediately after this delete succeeds, be doing
backup_.swap(backup)?

It looks as though, currently, a failure at lines 65 or 71 would leave the
registry messed up. I assume that the work item system invokes Rollback() on the
item that failed as well as previously successful items?

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item.cc:97: if (ignore_failure_)
Is it really up to each task to do this?

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item.cc:105: << " in rollback, result: "
<< result;
Is it intentional that we continue with the attempt to restore? I guess it's a
best-effort at that point?

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item.cc:108: // Restore the old
contents.  The restoration takes on its default security
This is also a nearly line-for-line duplicate of delete_reg_key_work_item.cc.
DRYer?

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
File chrome/installer/util/copy_reg_key_work_item.h (right):

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item.h:19: // that in the event of
rollback the key's values and subkeys are restored but
In the case of a rollback, why would the new key and subkeys still exist?
Presumably because this task can overwrite existing data? Might be worth
explicitly documenting its behaviour in this case.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item.h:23: ~CopyRegKeyWorkItem();
I think this destructor should explicitly be marked as virtual.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
File chrome/installer/util/copy_reg_key_work_item_unittest.cc (right):

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item_unittest.cc:18: // registry during
setup.  Each test runs
incomplete comment?

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item_unittest.cc:23: static void
SetUpTestCase() {
Unless there's a reason to use SetUpTestCase, I would suggest just using SetUp,
since it protects you against someone unknowingly adding a test that manipulates
the data outside of Destination.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item_unittest.cc:31: RegKey
key(kRootKey, L"", KEY_QUERY_VALUE);
This appears to be a common setup/teardown behaviour:

http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=%23i...

I wonder if it's not a worthwhile idea to define a RegKeyForTesting RAII class
that you could define as a member of the Test subclass and then initialize with
a path.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item_unittest.cc:54: delete
non_empty_key_path_;
All of these would go away if you used SetUp/TearDown, member instances instead
of member pointers, etc.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item_unittest.cc:63:
logging::CloseLogFile();
Why is this here?

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item_unittest.cc:191: }
I suppose completeness would dictate a test for Rollback() when ignore_failures_
== true.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/delete_r...
File chrome/installer/util/delete_reg_key_work_item.cc (right):

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/delete_r...
chrome/installer/util/delete_reg_key_work_item.cc:18: }
}  // namespace

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/registry...
File chrome/installer/util/registry_key_backup.cc (right):

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/registry...
chrome/installer/util/registry_key_backup.cc:18: }
}  // namespace

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/registry...
chrome/installer/util/registry_key_backup.cc:45:
RegistryKeyBackup::RegistryValueBackup::~RegistryValueBackup()
Why are you required to explicitly define a destructor?

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/registry...
File chrome/installer/util/registry_key_backup.h (right):

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/registry...
chrome/installer/util/registry_key_backup.h:19: }
}  // namespace win
}  // namespace base

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/registry...
File chrome/installer/util/registry_key_backup_unittest.cc (right):

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/registry...
chrome/installer/util/registry_key_backup_unittest.cc:20: static void
SetUpTestCase() {
Same questions apply about SetUpTestCase vs. SetUp

[grt (UTC plus 2), 2011-09-16 17:45:45]

Thanks for the excellent suggestions.  I believe I've addressed everything. 
PTAL.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
File chrome/installer/util/copy_reg_key_work_item.cc (right):

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item.cc:18: }
On 2011/09/15 18:38:57, erikwright wrote:
> }  // namespace
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces

Done.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item.cc:42: LONG result =
dest_key.Open(predefined_root_, dest_key_path_.c_str(),
On 2011/09/15 18:38:57, erikwright wrote:
> Any chance you can make this a little DRYer?
> 
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/installer/util...

That code was so damn sweet I had to do it twice, man!  Done.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item.cc:59: LONG result =
SHDeleteKey(predefined_root_, dest_key_path_.c_str());
On 2011/09/15 18:38:57, erikwright wrote:
> Should you not, immediately after this delete succeeds, be doing
> backup_.swap(backup)?
> 
> It looks as though, currently, a failure at lines 65 or 71 would leave the
> registry messed up. I assume that the work item system invokes Rollback() on
the
> item that failed as well as previously successful items?

Good catch.  Yes, the list will rollback the failed item, so this is definitely
the right thing to do.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item.cc:97: if (ignore_failure_)
On 2011/09/15 18:38:57, erikwright wrote:
> Is it really up to each task to do this?

Sadly, yes.  Overhauling WorkItemList is outside of the scope of this CL.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item.cc:105: << " in rollback, result: "
<< result;
On 2011/09/15 18:38:57, erikwright wrote:
> Is it intentional that we continue with the attempt to restore? I guess it's a
> best-effort at that point?

Yeah, I think it's consistent to go ahead and restore.  When rolling back a
WorkItemList, the failure of one item to rollback doesn't stop the rollback of
the list as a whole.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item.cc:108: // Restore the old
contents.  The restoration takes on its default security
On 2011/09/15 18:38:57, erikwright wrote:
> This is also a nearly line-for-line duplicate of delete_reg_key_work_item.cc.
> DRYer?

Done.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
File chrome/installer/util/copy_reg_key_work_item.h (right):

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item.h:19: // that in the event of
rollback the key's values and subkeys are restored but
On 2011/09/15 18:38:57, erikwright wrote:
> In the case of a rollback, why would the new key and subkeys still exist?
> Presumably because this task can overwrite existing data? Might be worth
> explicitly documenting its behaviour in this case.

Done.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item.h:23: ~CopyRegKeyWorkItem();
On 2011/09/15 18:38:57, erikwright wrote:
> I think this destructor should explicitly be marked as virtual.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...

Done.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
File chrome/installer/util/copy_reg_key_work_item_unittest.cc (right):

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item_unittest.cc:18: // registry during
setup.  Each test runs
On 2011/09/15 18:38:57, erikwright wrote:
> incomplete comment?

Done.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item_unittest.cc:23: static void
SetUpTestCase() {
On 2011/09/15 18:38:57, erikwright wrote:
> Unless there's a reason to use SetUpTestCase, I would suggest just using
SetUp,
> since it protects you against someone unknowingly adding a test that
manipulates
> the data outside of Destination.

It's a little more work and IO for each test to do that.  Given that test
runtime is a recurring issue for Chrome, I figured it's worth the code cost of
making this happen here.  On the other hand, since each test diddles the
registry anyway, it's probably a bad call.  Done.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item_unittest.cc:31: RegKey
key(kRootKey, L"", KEY_QUERY_VALUE);
On 2011/09/15 18:38:57, erikwright wrote:
> This appears to be a common setup/teardown behaviour:
> 
>
http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=%252...
> 
> I wonder if it's not a worthwhile idea to define a RegKeyForTesting RAII class
> that you could define as a member of the Test subclass and then initialize
with
> a path.

Done.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item_unittest.cc:54: delete
non_empty_key_path_;
On 2011/09/15 18:38:57, erikwright wrote:
> All of these would go away if you used SetUp/TearDown, member instances
instead
> of member pointers, etc.

Done.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item_unittest.cc:63:
logging::CloseLogFile();
On 2011/09/15 18:38:57, erikwright wrote:
> Why is this here?

Beats the hell out of me.  It's all over the unit tests in this directory. 
Monkey see, monkey do.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/copy_reg...
chrome/installer/util/copy_reg_key_work_item_unittest.cc:191: }
On 2011/09/15 18:38:57, erikwright wrote:
> I suppose completeness would dictate a test for Rollback() when
ignore_failures_
> == true.

Done.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/delete_r...
File chrome/installer/util/delete_reg_key_work_item.cc (right):

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/delete_r...
chrome/installer/util/delete_reg_key_work_item.cc:18: }
On 2011/09/15 18:38:57, erikwright wrote:
> }  // namespace

Done.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/registry...
File chrome/installer/util/registry_key_backup.cc (right):

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/registry...
chrome/installer/util/registry_key_backup.cc:18: }
On 2011/09/15 18:38:57, erikwright wrote:
> }  // namespace

Done.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/registry...
chrome/installer/util/registry_key_backup.cc:45:
RegistryKeyBackup::RegistryValueBackup::~RegistryValueBackup()
On 2011/09/15 18:38:57, erikwright wrote:
> Why are you required to explicitly define a destructor?

Since the class has non-POD data members, the dtor does real work.  Chromium has
a "there's too damn much inlining!" mandate going around, so the current advice
is to make ctors and dtors of such classes non-inline by defining them in the
.cc files; see
http://www.chromium.org/developers/coding-style#TOC-Inline-functions for more.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/registry...
File chrome/installer/util/registry_key_backup.h (right):

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/registry...
chrome/installer/util/registry_key_backup.h:19: }
On 2011/09/15 18:38:57, erikwright wrote:
> }  // namespace win
> }  // namespace base

Done.

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/registry...
File chrome/installer/util/registry_key_backup_unittest.cc (right):

http://codereview.chromium.org/7890069/diff/16/chrome/installer/util/registry...
chrome/installer/util/registry_key_backup_unittest.cc:20: static void
SetUpTestCase() {
On 2011/09/15 18:38:57, erikwright wrote:
> Same questions apply about SetUpTestCase vs. SetUp

Done.

[erikwright (departed), 2011-09-16 18:41:45]

Looks pretty good to me. One nit and two minor requests.

http://codereview.chromium.org/7890069/diff/12001/chrome/installer/util/delet...
File chrome/installer/util/delete_reg_key_work_item.cc (right):

http://codereview.chromium.org/7890069/diff/12001/chrome/installer/util/delet...
chrome/installer/util/delete_reg_key_work_item.cc:58: // Delete anything in the
key before restoring the backup in case someone else
Would anyone ever want to "WriteTo" without deleting?

Maybe "RestoreTo" and then incorporate the Delete inside?

Also, are the arguments to WriteTo() ever going to be different from the
arguments to Initialize().

http://codereview.chromium.org/7890069/diff/12001/chrome/installer/util/regis...
File chrome/installer/util/registry_key_backup.h (right):

http://codereview.chromium.org/7890069/diff/12001/chrome/installer/util/regis...
chrome/installer/util/registry_key_backup.h:58: namespace std {
This technically (at least) violates the coding guidelines, although I know that
it is considered appropriate for STL.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

I failed to find any code already doing this:

http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=%22n...

I think it might be best to do without if possible. Given that you defined a
member swap(), is it still required?

I guess the question begs to be asked, is it worth implementing swap over using
scoped_ptr as you previously did? I'm indifferent, I guess, though technically
defining your own implies writing a test case for your own.

http://codereview.chromium.org/7890069/diff/12001/chrome/installer/util/regis...
File chrome/installer/util/registry_test_data.h (right):

http://codereview.chromium.org/7890069/diff/12001/chrome/installer/util/regis...
chrome/installer/util/registry_test_data.h:7: 
#pragma once

[grt (UTC plus 2), 2011-09-16 20:44:46]

PTAL.

http://codereview.chromium.org/7890069/diff/12001/chrome/installer/util/delet...
File chrome/installer/util/delete_reg_key_work_item.cc (right):

http://codereview.chromium.org/7890069/diff/12001/chrome/installer/util/delet...
chrome/installer/util/delete_reg_key_work_item.cc:58: // Delete anything in the
key before restoring the backup in case someone else
On 2011/09/16 18:41:45, erikwright wrote:
> Would anyone ever want to "WriteTo" without deleting?
> 
> Maybe "RestoreTo" and then incorporate the Delete inside?
> 
> Also, are the arguments to WriteTo() ever going to be different from the
> arguments to Initialize().

I debated that, but decided that there is some utility in using
RegistryKeyBackup to suck data from point A and dump it into point B.  One may
not want to delete the destination.

http://codereview.chromium.org/7890069/diff/12001/chrome/installer/util/regis...
File chrome/installer/util/registry_key_backup.h (right):

http://codereview.chromium.org/7890069/diff/12001/chrome/installer/util/regis...
chrome/installer/util/registry_key_backup.h:58: namespace std {
On 2011/09/16 18:41:45, erikwright wrote:
> This technically (at least) violates the coding guidelines, although I know
that
> it is considered appropriate for STL.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

The style guide's guidance is overreaching in this case.  This definition into
std is appropriate since it is permitted to specialize templates in std.  C++-08
17.4.3.1: "A program may add template specializations for any
standard library template to namespace std."

> I failed to find any code already doing this:
> 
>
http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=%252...
> 
> I think it might be best to do without if possible. Given that you defined a
> member swap(), is it still required?

It allows callers to use the "standard" way of calling swap, where they don't
need to know or care whether or not the thing being swapped can do it smartly or
not.  Since it's not normal for Chromium, I'll yank it.  (I figured this would
stand out.  :-) )

> I guess the question begs to be asked, is it worth implementing swap over
using
> scoped_ptr as you previously did? I'm indifferent, I guess, though technically
> defining your own implies writing a test case for your own.

I think the swap itself is fine.  I'll remove the template specialization since
it isn't required (the caller in this case can invoke the method directly) and
add a unit test.

http://codereview.chromium.org/7890069/diff/12001/chrome/installer/util/regis...
File chrome/installer/util/registry_test_data.h (right):

http://codereview.chromium.org/7890069/diff/12001/chrome/installer/util/regis...
chrome/installer/util/registry_test_data.h:7: 
On 2011/09/16 18:41:45, erikwright wrote:
> #pragma once

Done.

[erikwright (departed), 2011-09-17 01:29:40]

LGTM++.

[commit-bot: I haz the power, 2011-09-17 16:38:05]

Can't apply patch for file chrome/installer/util/registry_key_backup.cc.
While running patch -p1 --forward --force;
patching file chrome/installer/util/delete_reg_key_work_item.cc
Hunk #1 FAILED at 1.
Hunk #2 FAILED at 117.
Hunk #3 FAILED at 140.
Hunk #4 FAILED at 166.
Hunk #5 FAILED at 228.
Hunk #6 FAILED at 245.
Hunk #7 FAILED at 264.
7 out of 7 hunks FAILED -- saving rejects to file
chrome/installer/util/delete_reg_key_work_item.cc.rej

[commit-bot: I haz the power, 2011-09-17 16:48:29]

Can't apply patch for file chrome/installer/util/registry_key_backup.cc.
While running patch -p1 --forward --force;
patching file chrome/installer/util/delete_reg_key_work_item.cc
Hunk #1 FAILED at 1.
Hunk #2 FAILED at 117.
Hunk #3 FAILED at 140.
Hunk #4 FAILED at 166.
Hunk #5 FAILED at 228.
Hunk #6 FAILED at 245.
Hunk #7 FAILED at 264.
7 out of 7 hunks FAILED -- saving rejects to file
chrome/installer/util/delete_reg_key_work_item.cc.rej

[commit-bot: I haz the power, 2011-09-17 17:03:58]

Can't apply patch for file chrome/installer/util/registry_key_backup.cc.
While running patch -p1 --forward --force;
patching file chrome/installer/util/delete_reg_key_work_item.cc
Hunk #1 FAILED at 1.
Hunk #2 FAILED at 117.
Hunk #3 FAILED at 140.
Hunk #4 FAILED at 166.
Hunk #5 FAILED at 228.
Hunk #6 FAILED at 245.
Hunk #7 FAILED at 264.
7 out of 7 hunks FAILED -- saving rejects to file
chrome/installer/util/delete_reg_key_work_item.cc.rej