Added CopyRegKeyWorkItem in support of IE low rights policy fixes.

chrome/installer/util/copy_reg_key_work_item_unittest.cc

Index: chrome/installer/util/copy_reg_key_work_item_unittest.cc
diff --git a/chrome/installer/util/copy_reg_key_work_item_unittest.cc b/chrome/installer/util/copy_reg_key_work_item_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..3db56f972fa8833584d459c126eef9a3f1f4c784
--- /dev/null
+++ b/chrome/installer/util/copy_reg_key_work_item_unittest.cc
@@ -0,0 +1,191 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <windows.h>
+#include <shlwapi.h>  // NOLINT
+
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/win/registry.h"
+#include "chrome/installer/util/copy_reg_key_work_item.h"
+#include "chrome/installer/util/work_item.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using base::win::RegKey;
+
+// Test fixture for CopyRegKeyWorkItem.  The fixture sets up some data in the
+// registry during setup.  Each test runs

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

incomplete comment?

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

Done.

+class CopyRegKeyWorkItemTest : public testing::Test {
+ protected:
+  static const HKEY kRootKey;
+
+  static void SetUpTestCase() {

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

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.

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

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.

+    // Make some strings that we'll use.
+    temp_key_path_ = new std::wstring(L"SOFTWARE\\TmpTmp");
+    dest_key_path_ = new std::wstring(*temp_key_path_ + L"\\Destination");
+    empty_key_path_ = new std::wstring(*temp_key_path_ + L"\\EmptyKey");
+    non_empty_key_path_ = new std::wstring(*temp_key_path_ + L"\\NonEmptyKey");
+
+    // Create a temporary key for testing
+    RegKey key(kRootKey, L"", KEY_QUERY_VALUE);

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

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.

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

Done.

+    key.DeleteKey(temp_key_path_->c_str());
+    ASSERT_NE(ERROR_SUCCESS,
+              key.Open(kRootKey, temp_key_path_->c_str(), KEY_READ));
+    ASSERT_EQ(ERROR_SUCCESS,
+              key.Create(kRootKey, temp_key_path_->c_str(), KEY_READ));
+
+    // Put some data in the registry
+    ASSERT_EQ(ERROR_SUCCESS,
+              key.Create(kRootKey, empty_key_path_->c_str(), KEY_READ));
+    ASSERT_EQ(ERROR_SUCCESS,
+              key.Create(kRootKey, non_empty_key_path_->c_str(), KEY_WRITE));
+    ASSERT_EQ(ERROR_SUCCESS,
+              key.WriteValue(NULL, non_empty_key_path_->c_str()));
+    ASSERT_EQ(ERROR_SUCCESS, key.CreateKey(L"Subkey", KEY_WRITE));
+    ASSERT_EQ(ERROR_SUCCESS, key.WriteValue(L"SomeValue", 1U));
+  }
+
+  static void TearDownTestCase() {
+    // Clean up the temporary key
+    RegKey key(kRootKey, L"", KEY_QUERY_VALUE);
+    ASSERT_EQ(ERROR_SUCCESS, key.DeleteKey(temp_key_path_->c_str()));
+
+    delete non_empty_key_path_;

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

All of these would go away if you used SetUp/TearDown, member instances instead
of member pointers, etc.

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

Done.

+    non_empty_key_path_ = NULL;
+    delete empty_key_path_;
+    empty_key_path_ = NULL;
+    delete dest_key_path_;
+    dest_key_path_ = NULL;
+    delete temp_key_path_;
+    temp_key_path_ = NULL;
+
+    logging::CloseLogFile();

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

Why is this here?

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

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

+  }
+
+  virtual void SetUp() {
+  }
+
+  virtual void TearDown() {
+    // Delete the destination for the next test.
+    RegKey key(kRootKey, L"", KEY_QUERY_VALUE);
+    LONG result = key.DeleteKey(dest_key_path_->c_str());
+    ASSERT_TRUE(result == ERROR_SUCCESS || result == ERROR_FILE_NOT_FOUND);
+  }
+
+  static std::wstring* temp_key_path_;
+  static std::wstring* dest_key_path_;
+  static std::wstring* empty_key_path_;
+  static std::wstring* non_empty_key_path_;
+};
+
+const HKEY CopyRegKeyWorkItemTest::kRootKey = HKEY_CURRENT_USER;
+std::wstring* CopyRegKeyWorkItemTest::temp_key_path_ = NULL;
+std::wstring* CopyRegKeyWorkItemTest::dest_key_path_ = NULL;
+std::wstring* CopyRegKeyWorkItemTest::empty_key_path_ = NULL;
+std::wstring* CopyRegKeyWorkItemTest::non_empty_key_path_ = NULL;
+
+// Test that copying a key that doesn't exist succeeds, and that rollback does
+// nothing.
+TEST_F(CopyRegKeyWorkItemTest, TestNoKey) {
+  const std::wstring key_paths[] = {
+    std::wstring(*temp_key_path_ + L"\\NoKeyHere"),
+    std::wstring(*temp_key_path_ + L"\\NoKeyHere\\OrHere")
+  };
+  RegKey key;
+  for (size_t i = 0; i < arraysize(key_paths); ++i) {
+    const std::wstring& key_path = key_paths[i];
+    scoped_ptr<CopyRegKeyWorkItem> item(
+        WorkItem::CreateCopyRegKeyWorkItem(kRootKey, key_path,
+                                           *dest_key_path_));
+    EXPECT_TRUE(item->Do());
+    EXPECT_NE(ERROR_SUCCESS,
+              key.Open(kRootKey, dest_key_path_->c_str(), KEY_READ));
+    item->Rollback();
+    item.reset();
+    EXPECT_NE(ERROR_SUCCESS,
+              key.Open(kRootKey, dest_key_path_->c_str(), KEY_READ));
+  }
+}
+
+// Test that copying an empty key succeeds, and that rollback removes the copy.
+TEST_F(CopyRegKeyWorkItemTest, TestEmptyKey) {
+  RegKey key;
+  scoped_ptr<CopyRegKeyWorkItem> item(
+      WorkItem::CreateCopyRegKeyWorkItem(kRootKey, *empty_key_path_,
+                                         *dest_key_path_));
+  EXPECT_TRUE(item->Do());
+  EXPECT_EQ(ERROR_SUCCESS,
+            key.Open(kRootKey, dest_key_path_->c_str(), KEY_READ));
+  item->Rollback();
+  item.reset();
+  EXPECT_NE(ERROR_SUCCESS,
+            key.Open(kRootKey, dest_key_path_->c_str(), KEY_READ));
+}
+
+// Test that copying a key with subkeys and values succeeds, and that rollback
+// removes the copy.
+TEST_F(CopyRegKeyWorkItemTest, TestNonEmptyKey) {
+  RegKey key;
+  scoped_ptr<CopyRegKeyWorkItem> item(
+      WorkItem::CreateCopyRegKeyWorkItem(kRootKey, *non_empty_key_path_,
+                                         *dest_key_path_));
+  EXPECT_TRUE(item->Do());
+  EXPECT_EQ(ERROR_SUCCESS,
+            key.Open(kRootKey, dest_key_path_->c_str(), KEY_READ));
+  std::wstring str_value;
+  EXPECT_EQ(ERROR_SUCCESS, key.ReadValue(NULL, &str_value));
+  EXPECT_EQ(*non_empty_key_path_, str_value);
+  EXPECT_EQ(ERROR_SUCCESS, key.OpenKey(L"Subkey", KEY_READ));
+  DWORD dw_value = 0;
+  EXPECT_EQ(ERROR_SUCCESS, key.ReadValueDW(L"SomeValue", &dw_value));
+  EXPECT_EQ(1U, dw_value);
+  item->Rollback();
+  item.reset();
+  EXPECT_NE(ERROR_SUCCESS,
+            key.Open(kRootKey, dest_key_path_->c_str(), KEY_READ));
+}
+
+// Test that copying an empty key over a key with data succeeds, and that
+// rollback restores the original data.
+TEST_F(CopyRegKeyWorkItemTest, TestOverwriteAndRestore) {
+  RegKey key;
+  // First copy the data into the dest.
+  EXPECT_EQ(ERROR_SUCCESS,
+            key.Create(kRootKey, dest_key_path_->c_str(), KEY_WRITE));
+  EXPECT_EQ(ERROR_SUCCESS,
+            SHCopyKey(kRootKey, non_empty_key_path_->c_str(), key.Handle(), 0));
+  key.Close();
+
+  // Now copy the empty key into the dest.
+  scoped_ptr<CopyRegKeyWorkItem> item(
+      WorkItem::CreateCopyRegKeyWorkItem(kRootKey, *empty_key_path_,
+                                         *dest_key_path_));
+  EXPECT_TRUE(item->Do());
+
+  // Make sure the dest is now empty.
+  DWORD num_subkeys = 0;
+  DWORD num_values = 0;
+  EXPECT_EQ(ERROR_SUCCESS,
+            key.Open(kRootKey, dest_key_path_->c_str(), KEY_READ));
+  EXPECT_EQ(ERROR_SUCCESS,
+            RegQueryInfoKey(key.Handle(), NULL, NULL, NULL, &num_subkeys,
+                            NULL, NULL, &num_values, NULL, NULL, NULL, NULL));
+  EXPECT_EQ(0UL, num_subkeys);
+  EXPECT_EQ(0UL, num_values);
+  key.Close();
+
+  // Restore the data.
+  item->Rollback();
+  item.reset();
+
+  EXPECT_EQ(ERROR_SUCCESS,
+            key.Open(kRootKey, dest_key_path_->c_str(), KEY_READ));
+  std::wstring str_value;
+  EXPECT_EQ(ERROR_SUCCESS, key.ReadValue(NULL, &str_value));
+  EXPECT_EQ(*non_empty_key_path_, str_value);
+  EXPECT_EQ(ERROR_SUCCESS, key.OpenKey(L"Subkey", KEY_READ));
+  DWORD dw_value = 0;
+  EXPECT_EQ(ERROR_SUCCESS, key.ReadValueDW(L"SomeValue", &dw_value));
+  EXPECT_EQ(1U, dw_value);
+}

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

I suppose completeness would dictate a test for Rollback() when ignore_failures_
== true.

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

Done.