Bump os-upgrade component of Active Setup version on-os-upgrade.

chrome/installer/util/install_util.cc

Index: chrome/installer/util/install_util.cc
diff --git a/chrome/installer/util/install_util.cc b/chrome/installer/util/install_util.cc
index f7dccea263c8bc2f0c43b040dd191b70324d294e..32c794d130069d3c11b837af1f1177eb6ab875b7 100644
--- a/chrome/installer/util/install_util.cc
+++ b/chrome/installer/util/install_util.cc
@@ -19,6 +19,9 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
 #include "base/process/launch.h"
+#include "base/strings/string16.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/sys_info.h"
@@ -128,6 +131,63 @@ HWND CreateUACForegroundWindow() {
 
 }  // namespace
 
+base::string16 InstallUtil::GetActiveSetupVersionFromExisting(
+    ActiveSetupVersionComponent component,
+    ActiveSetupVersionOperation operation,
+    int value,
+    const base::string16& existing_version) {
+  std::vector<base::string16> version_components = base::SplitString(

[grt (UTC plus 2), 2015/07/01 15:59:14]

#include <vector>

[gab, 2015/07/02 03:50:43]

Done.

+      existing_version, L",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+
+  // If |existing_version| was empty or otherwise corrupted, turn it into a
+  // valid one.
+  if (version_components.size() != 4U)
+    version_components.assign(4U, L"0");
+
+  switch (operation) {
+    case SET:
+      version_components[component] = base::IntToString16(value);
+      break;
+    case BUMP:
+      int previous_value;
+      if (base::StringToInt(version_components[component], &previous_value)) {
+        version_components[component] =
+            base::IntToString16(previous_value + value);
+      }
+      break;
+  }
+
+  return JoinString(version_components, L',');
+}
+
+bool InstallUtil::UpdateActiveSetupVersion(
+    BrowserDistribution* dist,
+    ActiveSetupVersionComponent component,
+    ActiveSetupVersionOperation operation,
+    int value) {
+  base::string16 active_setup_reg(GetActiveSetupPath(dist));
+  base::win::RegKey active_setup_key(

[grt (UTC plus 2), 2015/07/01 15:59:14]

this will create the key if it doesn't exist. i don't think this is what you
really want here since there's no value to update in that case. i think it's
better to use:
  base::win::RegKey active_setup_key;
  if (active_setup_key.Open(...) != ERROR_SUCCESS) {
    ...log and return false;
  }

[gab, 2015/07/02 03:50:43]

Done.

+      HKEY_LOCAL_MACHINE, active_setup_reg.c_str(),
+      KEY_QUERY_VALUE | KEY_SET_VALUE | KEY_WOW64_32KEY);

[grt (UTC plus 2), 2015/07/01 15:59:14]

AddActiveSetupWorkItems uses kWow64Default, so remove KEY_WOW64_32KEY here.

[gab, 2015/07/02 03:50:43]

Done.

+  if (!active_setup_key.Valid()) {
+    VLOG(1) << "Active Setup key invalid.";

[grt (UTC plus 2), 2015/07/01 15:59:13]

LOG(ERROR) here and line 180? if not, why?

[gab, 2015/07/02 03:50:43]

I figured the callee already logs an error if this returns false and the detail
as to exactly what failed is perhaps more in the realm of "verbose-logging"?
WDYT>

+    return false;
+  }
+
+  base::string16 existing_version;
+  if (active_setup_key.ReadValue(L"Version",
+                                 &existing_version) != ERROR_SUCCESS) {
+    VLOG(1) << "Unable to read Active Setup key to string.";
+    return false;
+  }
+
+  base::string16 updated_version = GetActiveSetupVersionFromExisting(

[grt (UTC plus 2), 2015/07/01 15:59:14]

if |existing_version| is somehow malformed such that the split on line 139
doesn't satisfy the condition on 144, i think the right thing to do is for it to
end up being set to kActiveSetupMajorVersion,0,1,0 rather than 0,0,1,0.

[gab, 2015/07/02 03:50:43]

I thought about that too, not sure how to structure the API for this to fit
nicely though. As-is 43,0,1,0 would eventually come true on the next auto-update
-- do we think corruption is likely enough to care about a more complex solution
that supports immediate cleanup (I guess the beacons timing and potential
automatic shortcut installs on-os-upgrade become can of weird if we let this
happen..)? This is where I would potentially see the need to extract this code
to its own component that manages ALL of the Active Setup version logic...

+      component, operation, value, existing_version);
+
+  return active_setup_key.WriteValue(L"Version", updated_version.c_str()) ==
+         ERROR_SUCCESS;

[grt (UTC plus 2), 2015/07/01 15:59:14]

is this how git cl format does it? i think this should be 4-space indent, not
aligned with "active_setup_key".

[gab, 2015/07/02 03:50:43]

Yea this is git cl format (I agree with you but this didn't feel ugly enough for
me to turn down the change it made -- whereas I did turn it down when it did the
same thing on 178-179 above, putting ReadValue(x,y) on a single line ending with
"!=" with ERROR_SUCCESS aligned flush on the next line...).

+}
+
 base::string16 InstallUtil::GetActiveSetupPath(BrowserDistribution* dist) {
   static const wchar_t kInstalledComponentsPath[] =
       L"Software\\Microsoft\\Active Setup\\Installed Components\\";