Beacons for tracking default browser status.

chrome/installer/util/beacons.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_BEACONS_H_
+#define CHROME_INSTALLER_UTIL_BEACONS_H_
+
+#include <windows.h>
+
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/strings/string16.h"
+#include "base/strings/string_piece.h"
+#include "base/time/time.h"
+#include "chrome/installer/util/shell_util.h"
+
+class AppRegistrationData;
+class BrowserDistribution;
+
+namespace base {
+class FilePath;
+}
+
+// Checks the default state of the browser for the current user and updates the
+// appropriate beacon (last was default or first not default).

[gab, 2015/05/26 18:54:34]

Reorder sentence to begin with the actual action this method performs?
i.e.:

Updates the (last was default or first not default) beacon based on the default
state of the browser for the current user.

?

(I'm asking specifically because it confused me when first glancing at
definitions that the two first methods were "Update...()" yet their descriptions
began differently by "Checks" and "Updates)

[grt (UTC plus 2), 2015/05/26 20:54:01]

It is in the right order: it checks the default state and updates the beacons.

[gab, 2015/05/29 13:37:36]

Well the fact that it "checks" is a side-effect of what the implementation has
to do, but what it really does is ensures the update has occurred (in fact the
check is such an implementation detail that the result is ignored).

[grt (UTC plus 2), 2015/05/29 15:31:10]

The only way that the beacons can be updated to reflect the defaultness of
Chrome is to check whether or not Chrome is the default browser. This is
orthogonal to the specific way it is implemented. Since the beacon update is
buried at the lowest layer of Chrome's "am I default?" code, the implementation
calls down to that code and ignores the result. It still remains that,
semantically speaking, this function must a) somehow check to see if Chrome is
the default browser, and b) update the beacons accordingly. It just so happens
that b is a consequence of the implementation of a.

+void UpdateDefaultBrowserBeaconForPath(const base::FilePath& chrome_exe);
+
+// Updates the last was default or first not default beacon for the current user
+// based on |default_state|.
+void UpdateDefaultBrowserBeacon(const base::FilePath& chrome_exe,

[gab, 2015/05/26 18:54:34]

s/UpdateDefaultBrowserBeacon/UpdateDefaultBrowserBeaconWithState ?

i.e., make it more obvious what the difference between this one and the other
is?

[grt (UTC plus 2), 2015/05/26 20:54:01]

Done.

+                                BrowserDistribution* distribution,
+                                ShellUtil::DefaultState default_state);
+
+// Updates the last OS upgrade beacon for the install.
+void UpdateOsUpgradeBeacon(bool system_install,
+                           BrowserDistribution* distribution);
+
+// A named beacon stored in the registry representing the first or last time at
+// which some event took place. A beacon may apply to a per-user event or a
+// per-install event.
+class Beacon {

[gab, 2015/05/26 18:54:34]

I think this should either be in the install_util namespace or have a more
specific name than simply "Beacon".

[grt (UTC plus 2), 2015/05/26 20:54:01]

I've been steered away from namespaces by other reviewers. Could you suggest a
name you'd be happy with?

[grt (UTC plus 2), 2015/05/28 14:05:42]

Thinking about it more, this class is pretty much an implementation detail for
the functions above, so I think it does make sense to move it into the
installer_util namespace (leaving the functions outside of the namespace). SGTY?

[gab, 2015/05/29 13:37:36]

I like a namespace here; otherwise you need a more Installer specific name for
this class anyways. Either way you need a way to make this name more specific, a
namespace seems right to me.

[grt (UTC plus 2), 2015/05/29 15:31:10]

Done.

+ public:
+  ~Beacon();

[gab, 2015/05/26 18:54:34]

I think you can use
~Beacon() = default;
here instead of having the definition in the .cc (I'm not sure whether that's
the same as inlining it, but either way you should be fine as your class is only
made of POD types (+ strings which IIRC are also considered POD in some way))

[grt (UTC plus 2), 2015/05/26 20:54:01]

A string16 isn't a POD since it has a dtor, but maybe it's considered trivial
enough to not call for an out-of-line dtor here. I find the rules around this
somewhat arbitrary, and I'm loath to change it without knowing that it isn't
going to set off an alarm somewhere. I'm trying to build with clang, but clang
itself isn't building for me at the moment. I'll leave this as-is for now.

+
+  // Returns an instance representing the last time the machine's OS was
+  // ugpraded.
+  static scoped_ptr<Beacon> LastOsUpgrade(
+      bool system_install,
+      const AppRegistrationData& registration_data);
+
+  // Returns an instance representing the last time Chrome was the user's
+  // default browser.
+  static scoped_ptr<Beacon> LastWasDefault(
+      bool system_install,
+      const AppRegistrationData& registration_data);
+
+  // Returns an instance representing the first time Chrome was not the user's
+  // default browser.
+  static scoped_ptr<Beacon> FirstNotDefault(
+      bool system_install,
+      const AppRegistrationData& registration_data);
+
+  // Updates the beacon. For a type LAST beacon, the current time is written
+  // unconditionally. For a type FIRST beacon, the beacon is only updated if it
+  // does not already exist.
+  void Update();
+
+  // Removes the beacon.
+  void Remove();
+
+  // Returns the beacon's value or a null time if not found.
+  base::Time Get();
+
+ protected:
+  enum class BeaconType {
+    // A beacon that marks the first occurrence of an event.
+    FIRST,
+    // A beacon that marks the last occurrence of an event.
+    LAST,
+  };
+
+  enum class BeaconScope {
+    // A beacon that applies to a per-user event.
+    PER_USER,
+    // A beacon that applies to a per-install event.
+    PER_INSTALL,
+  };
+
+  Beacon(base::StringPiece16 name,
+         BeaconType type,
+         BeaconScope scope,
+         bool system_install,
+         const AppRegistrationData& registration_data);
+
+ private:
+  // Initializes the key_path_ and value_name_ fields of the beacon.
+  void Initialize(base::StringPiece16 name,
+                  bool system_install,
+                  const AppRegistrationData& registration_data);
+
+  // The type of beacon.
+  const BeaconType type_;
+
+  // The root key in the registry where this beacon is stored.
+  const HKEY root_;
+
+  // The scope of the beacon.
+  const BeaconScope scope_;
+
+  // The path to the registry key holding the beacon.
+  base::string16 key_path_;
+
+  // The name of the registry value holding the beacon.
+  base::string16 value_name_;
+
+  DISALLOW_COPY_AND_ASSIGN(Beacon);
+};
+
+#endif  // CHROME_INSTALLER_UTIL_BEACONS_H_