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

chrome/installer/util/app_command.cc

 // 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 "chrome/installer/util/app_command.h"
 
 #include "base/logging.h"
 #include "base/win/registry.h"
 #include "chrome/installer/util/google_update_constants.h"
 #include "chrome/installer/util/work_item_list.h"
 
 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) {
   if (!key.Valid()) {
     LOG(DFATAL) << "Cannot initialize an AppCommand from an invalid key.";
     return false;
   }
 
   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) {
     LOG(WARNING) << "Error reading " << google_update::kRegCommandLineField
                  << " value from registry: " << result;
     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