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

chrome/installer/util/install_util.cc

 // Copyright (c) 2012 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.
 //
 // See the corresponding header file for description of the functions in this
 // file.
 
 #include "chrome/installer/util/install_util.h"
 
 #include <shellapi.h>
 #include <shlobj.h>
 #include <shlwapi.h>
 
 #include <algorithm>
 
 #include "base/command_line.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #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"
 #include "base/values.h"
 #include "base/version.h"
 #include "base/win/metro.h"
 #include "base/win/registry.h"
 #include "base/win/windows_version.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
@@ skipping 89 matching lines @@
     } else {
       NOTREACHED() << "Unable to get default monitor";
     }
     ::SetForegroundWindow(foreground_window);
   }
   return foreground_window;
 }
 
 }  // 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\\";
   return kInstalledComponentsPath + dist->GetActiveSetupGuid();
 }
 
 void InstallUtil::TriggerActiveSetupCommand() {
   base::string16 active_setup_reg(
       GetActiveSetupPath(BrowserDistribution::GetDistribution()));
   base::win::RegKey active_setup_key(
@@ skipping 510 matching lines @@
   // Open the program and see if it references the expected file.
   base::File file;
   BY_HANDLE_FILE_INFORMATION info = {};
 
   return (OpenForInfo(path, &file) &&
           GetInfo(file, &info) &&
           info.dwVolumeSerialNumber == file_info_.dwVolumeSerialNumber &&
           info.nFileIndexHigh == file_info_.nFileIndexHigh &&
           info.nFileIndexLow == file_info_.nFileIndexLow);
 }