Callback flow to register Chrome and update shortcuts after OS upgrade to Windows 8

chrome/installer/util/app_command.cc

Index: chrome/installer/util/app_command.cc
diff --git a/chrome/installer/util/app_command.cc b/chrome/installer/util/app_command.cc
index 588fe86fda307d9615af880aee0c6813d1e3aa1f..1b3cd20894698aabb90d463e627395a9732f1624 100644
--- a/chrome/installer/util/app_command.cc
+++ b/chrome/installer/util/app_command.cc
@@ -11,17 +11,45 @@
 
 namespace installer {
 
+namespace {
+
+// Helper: Add an optional boolean registry value, specified by value_data.

[grt (UTC plus 2), 2012/08/30 04:15:25]

The sentence beginning with "In the registry, ..." is documenting Google Update
behavior, not Windows registry behavior, which is how it reads.  I don't think
this adds anything useful.  I propose replacing this whole comment with
something simple like:
"Adds a work item to set |value_name| to DWORD 1 if |value_data| is true; adds a
work item to remove |value_name| otherwise."

[huangs, 2012/08/30 17:13:07]

Done.

+// In the registry, a non-zero value is "true", and a missing or zero value
+// is "false". If value_data == true, add a SetRegValueWorkItem that sets
+// registry value of 1 with REG_DWORD type.  Otherwise add a
+// DeleteRegValueWorkItem that deletes the registry value.
+WorkItem* AddSetOptionalBoolRegValueWorkItem(
+    WorkItemList* item_list,

[grt (UTC plus 2), 2012/08/30 04:15:25]

chromium style: out params go at the end

[huangs, 2012/08/30 17:13:07]

Done.

+    HKEY predefined_root,
+    const string16& key_path,
+    const string16& value_name,
+    bool value_data) {
+  if (value_data) {
+    return item_list->AddSetRegValueWorkItem(predefined_root,

[grt (UTC plus 2), 2012/08/30 04:15:25]

get rid of the cast by adding:
    const DWORD kOne = 1;
before line 28 and replace "static_cast<DWORD>(1)" with "kOne"

[gab, 2012/08/30 14:24:32]

Interesting, I've never it this way in Chromium, but I see (and have been
suggested before) static_cast<DWORD>(1) all over the place. Isn't the compiler
smart enough to do this at compiler time anyways? Seems more concise then a new
variable...

[grt (UTC plus 2), 2012/08/30 17:06:08]

from the compiler's point of view, the two are equivalent.  from a code style
point of view, casts are the work of the devil and should be avoided when
possible.  in this case, it's possible to avoid the cast with no cost from the
POV of performance/code size/etc, and with a net increase in readability.

[huangs, 2012/08/30 17:13:07]

Done.

[huangs, 2012/08/30 17:13:07]

Conflicting reviews...  Going with my personal preference of with
"static_cast<DWORD>(1)", for shorter code.

[gab, 2012/08/30 17:15:50]

I disagree that this is more readbable, when one sees "kOne" he has to read up
to see what "kOne" is instead of seeing it inline... I don't see what's wrong
with a cast, to me, in this case, it's just a way of initializing a variable to
particular POD time inline.

But let's not make a second round of reviews just for this.

[grt (UTC plus 2), 2012/08/30 19:10:18]

i disagree.  in my religious text, good code has no casts.

+                                             key_path,
+                                             value_name,
+                                             static_cast<DWORD>(1),
+                                             true);
+  } else {
+    return item_list->AddDeleteRegValueWorkItem(predefined_root,
+                                                key_path,
+                                                value_name);
+  }
+}
+
+}  // namespace
+
 AppCommand::AppCommand()
     : sends_pings_(false),
-      is_web_accessible_(false) {
+      is_web_accessible_(false),
+      is_auto_run_on_os_upgrade_(false) {
 }
 
-AppCommand::AppCommand(const std::wstring& command_line,
-                       bool sends_pings,
-                       bool is_web_accessible)
+AppCommand::AppCommand(const string16& command_line)
     : command_line_(command_line),
-      sends_pings_(sends_pings),
-      is_web_accessible_(is_web_accessible) {
+      sends_pings_(false),
+      is_web_accessible_(false),
+      is_auto_run_on_os_upgrade_(false) {
 }
 
 bool AppCommand::Initialize(const base::win::RegKey& key) {
@@ -31,9 +59,10 @@ bool AppCommand::Initialize(const base::win::RegKey& key) {
   }
 
   LONG result = ERROR_SUCCESS;
-  std::wstring cmd_line;
+  string16 cmd_line;
   DWORD sends_pings = 0;
   DWORD is_web_acc = 0;
+  DWORD is_auto_run_on_os_upgrade = 0;
 
   result = key.ReadValue(google_update::kRegCommandLineField, &cmd_line);
   if (result != ERROR_SUCCESS) {
@@ -42,47 +71,48 @@ bool AppCommand::Initialize(const base::win::RegKey& key) {
     return false;
   }
 
+  // For the following calls, if result != ERROR_SUCCESS, then the

[grt (UTC plus 2), 2012/08/30 04:15:25]

I don't think this comment is needed.  If you feel otherwise, please simplify it
to something like:
// Note: ReadValueDW only modifies its out param on success.

[huangs, 2012/08/30 17:13:07]

Done.  So refer to functions by "ReadValueDW", not "ReadValueDW()"!

+  // value of the output variable (e.g. sends_pings) remains at 0.
   result = key.ReadValueDW(google_update::kRegSendsPingsField, &sends_pings);

[grt (UTC plus 2), 2012/08/30 04:15:25]

remove "result = " here and below

[huangs, 2012/08/30 17:13:07]

Done (embarrassed).

-  if (result != ERROR_SUCCESS) {
-    LOG(WARNING) << "Error reading " << google_update::kRegSendsPingsField
-                 << " value from registry: " << result;
-    return false;
-  }
-
   result = key.ReadValueDW(google_update::kRegWebAccessibleField, &is_web_acc);
-  if (result != ERROR_SUCCESS) {
-    LOG(WARNING) << "Error reading " << google_update::kRegWebAccessibleField
-                 << " value from registry: " << result;
-    return false;
-  }
+  result = key.ReadValueDW(google_update::kRegAutoRunOnOSUpgrade,
+                           &is_auto_run_on_os_upgrade);
 
   command_line_.swap(cmd_line);
   sends_pings_ = (sends_pings != 0);
   is_web_accessible_ = (is_web_acc != 0);
+  is_auto_run_on_os_upgrade_ = (is_auto_run_on_os_upgrade != 0);
 
   return true;
 }
 
 void AppCommand::AddWorkItems(HKEY predefined_root,
-                              const std::wstring& command_path,
+                              const string16& command_path,
                               WorkItemList* item_list) const {
-  const DWORD sends_pings = sends_pings_ ? 1U : 0U;
-  const DWORD is_web_accessible = is_web_accessible_ ? 1U : 0U;
-
   item_list->AddCreateRegKeyWorkItem(predefined_root, command_path)
-      ->set_log_message("creating quick-enable-cf command registry key");
+      ->set_log_message("creating AppCommand command registry key");

[grt (UTC plus 2), 2012/08/30 04:15:25]

"AppCommand command" -> "AppCommand"

[huangs, 2012/08/30 17:13:07]

Done.

   item_list->AddSetRegValueWorkItem(predefined_root, command_path,
                                     google_update::kRegCommandLineField,
                                     command_line_, true)
-      ->set_log_message("setting quick-enable-cf CommandLine registry value");
-  item_list->AddSetRegValueWorkItem(predefined_root, command_path,
-                                    google_update::kRegSendsPingsField,
-                                    sends_pings, true)
-      ->set_log_message("setting quick-enable-cf SendsPings registry value");
-  item_list->AddSetRegValueWorkItem(predefined_root, command_path,
-                                    google_update::kRegWebAccessibleField,
-                                    is_web_accessible, true)
-      ->set_log_message("setting quick-enable-cf WebAccessible registry value");
+      ->set_log_message("setting AppCommand CommandLine registry value");
+  AddSetOptionalBoolRegValueWorkItem(item_list,
+                                     predefined_root,
+                                     command_path,
+                                     google_update::kRegSendsPingsField,
+                                     sends_pings_)
+      ->set_log_message("setting AppCommand SendsPings registry value");
+  AddSetOptionalBoolRegValueWorkItem(item_list,
+                                     predefined_root,
+                                     command_path,
+                                     google_update::kRegWebAccessibleField,
+                                     is_web_accessible_)
+      ->set_log_message("setting AppCommand WebAccessible registry value");
+  AddSetOptionalBoolRegValueWorkItem(item_list,
+                                     predefined_root,
+                                     command_path,
+                                     google_update::kRegAutoRunOnOSUpgrade,
+                                     is_auto_run_on_os_upgrade_)
+      ->set_log_message("setting AppCommand AutoRunOnOSUpgrade registry value");
 }
 
 }  // namespace installer