Introduce UpdateActiveSetupVersionWorkItem

chrome/installer/util/update_active_setup_version_work_item.h

+// Copyright 2015 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.
+
+#ifndef CHROME_INSTALLER_UTIL_UPDATE_ACTIVE_SETUP_VERSION_WORK_ITEM_H_
+#define CHROME_INSTALLER_UTIL_UPDATE_ACTIVE_SETUP_VERSION_WORK_ITEM_H_
+
+#include "base/macros.h"
+#include "base/strings/string16.h"
+#include "chrome/installer/util/set_reg_value_work_item.h"
+
+// A WorkItem that updates (or installs if not present) the Active Setup
+// "Version" field in the registry. Optionally bumping the OS_UPGRADES component
+// on demand. This WorkItem is only viable on machine-wide installs.
+// TODO(gab): This would be cleaner if UpdateActiveSetupVersionWorkItem

[grt (UTC plus 2), 2015/07/09 14:23:53]

since you're modifying SetRegValueWorkItem to make this inheritance possible,
why not friend UpdateActiveSetupVersionWorkItem instead? i think it's cleaner to
use composition rather than inheritance here, even if it means poking an ugly
hole in a strange design.

[gab, 2015/07/09 20:36:38]

The reason I didn't do this is that friend'ing gives full access to the class
(i.e. there is no such thing as a "protected-only friend"), therefore I thought
this approach gave better encapsulation by only allowing the constructors to be
used.

[grt (UTC plus 2), 2015/07/10 15:37:27]

I see your point, but I think that's less bad than inheritance here.

[gab, 2015/07/13 16:37:38]

In that case, I think moving the constructors to public is better than
friend'ing.

+// embedded a SetRegValueWorkItem instead of being one, but this is not possible
+// in today's "private constructor + creator friends" WorkItem model :-(.
+class UpdateActiveSetupVersionWorkItem : public SetRegValueWorkItem {
+ public:
+  // The operation to be performed by this UpdateActiveSetupVersionWorkItem.
+  enum Operation {

[grt (UTC plus 2), 2015/07/09 14:23:53]

why not enum class?

[gab, 2015/07/09 20:36:38]

Only because it makes constructor calls even more wordy (though I generally like
the explicitness of enum class in general, happy either way, your pick!).

[grt (UTC plus 2), 2015/07/10 15:37:27]

Nah, I was just curious. Enum class does require a lot of typing, especially for
nested types like this.

+    // Update (or install if not present) the Active Setup "Version" in the
+    // registry.
+    UPDATE,
+    // Also bump the OS_UPGRADES component on top of updating the version
+    // (will default to 1 if the version was absent or invalid).
+    UPDATE_AND_BUMP_OS_UPGRADES_COMPONENT,
+  };
+
+  // Constructs an UpdateActiveSetupVersionWorkItem that will perform
+  // |operation_| on the |active_setup_path| key in the registry. This key needs

[grt (UTC plus 2), 2015/07/09 14:23:53]

nit: |operation|

[gab, 2015/07/09 20:36:38]

Done.

+  // to exist when this WorkItem is ran.
+  UpdateActiveSetupVersionWorkItem(const base::string16& active_setup_path,

[grt (UTC plus 2), 2015/07/09 14:23:53]

nit: make order of params and order of description in the doc comment match?

[gab, 2015/07/09 20:36:38]

I think the callsites read better in this order yet semantically I find the
comment reads better this way... Let me know if you feel otherwise.

[grt (UTC plus 2), 2015/07/10 15:37:27]

Only saying because I think I've been asked to do this (perhaps by you :-)). I
don't feel strongly.

[gab, 2015/07/13 16:37:38]

Possible :-), it's kind of arbitrary, but here I prefer it this way I think..

+                                   Operation operation);
+
+ private:
+  // The components of the Active Setup Version entry, in order.
+  enum ActiveSetupVersionComponent {
+    // The major version.
+    MAJOR,
+    // Unused component, always 0 for now.
+    UNUSED1,
+    // Number of OS upgrades handled since original install.

[grt (UTC plus 2), 2015/07/09 14:23:53]

should this warp back to zero each time the major version is incremented? is
there value in making this eternally increase rather than only being incremental
blips when MAJOR doesn't change?

[gab, 2015/07/09 20:36:38]

Interesting, no major reason other than that this would make the code slightly
more complex for no added benefit I think (plus in the event we want to know the
absolute number of OS major upgrades a user went through one day for whatever
reason, we have it..)

[gab, 2015/07/09 20:46:10]

Actually just thought of a reason, in my upcoming CL to restore shortcuts
on-os-upgrade I keep a cache of the last OS_UPGRADES handled by this user in
order to know when Active Setup kicks in whether it's kicking in response to an
OS_UPGRADES bump or something else.

If the major version gets updated between an OS upgrade and the next time a user
logs in, we still need to handle the OS upgrade.

+    OS_UPGRADES,
+    // Unused component, always 0 for now.
+    UNUSED2,
+  };
+
+  // Returns the updated Active Setup version to be used based on the
+  // |existing_version|.
+  base::string16 GetUpdatedActiveSetupVersion(
+      const base::string16& existing_version);
+
+  // The Operation to be performed by this WorkItem when executed.
+  const Operation operation_;
+
+  DISALLOW_COPY_AND_ASSIGN(UpdateActiveSetupVersionWorkItem);
+};
+
+#endif  // CHROME_INSTALLER_UTIL_UPDATE_ACTIVE_SETUP_VERSION_WORK_ITEM_H_