Beacons for tracking default browser status.

chrome/installer/util/beacons.cc

Index: chrome/installer/util/beacons.cc
diff --git a/chrome/installer/util/beacons.cc b/chrome/installer/util/beacons.cc
new file mode 100644
index 0000000000000000000000000000000000000000..00c12c93428d638d30ccac0a48eff995b3c32985
--- /dev/null
+++ b/chrome/installer/util/beacons.cc
@@ -0,0 +1,153 @@
+// 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.
+
+#include "chrome/installer/util/beacons.h"
+
+#include "base/win/registry.h"
+#include "base/win/win_util.h"
+#include "chrome/installer/util/app_registration_data.h"
+#include "chrome/installer/util/browser_distribution.h"
+#include "chrome/installer/util/install_util.h"
+#include "chrome/installer/util/shell_util.h"
+
+void UpdateDefaultBrowserBeaconForPath(const base::FilePath& chrome_exe) {
+  ignore_result(ShellUtil::GetChromeDefaultStateFromPath(chrome_exe));

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

Not done reading this CL but I assume this means that this ends up calling back
into UpdateDefaultBrowserBeacon()? This is non-obvious to a reader at this point
(and also fragile to the implementation of an external component :-(...).

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

Yes. The doc comment explains that it makes a check for defaultness. This
function is simply a wrapper around Chrome's existing "am I default?" check.
I chose to put the beacon update into the ShellUtil code that does Chrome's
default check since the beacon's reason for existence is to record the
last/first time Chrome saw that it was/was not default. Putting it anywhere else
would, in my opinion, put it too high up in the stack and require slapping
"update the beacon" code at each point where Chrome checks for defaultness
(settings page, startup, etc).

What function name/comments would you like to see to make this more clear?

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

How about
s/GetChromeDefaultStateFromPath/GetChromeDefaultStateFromPathAndCacheResult/ and
updating the ShellUtil method's description (i.e. it feels wrong to have a
GetFoo() method have a permanent side-effect and even more so to call
ignore_result(GetFoo()) from an external component solely to trigger that
side-effect.

With the above change we could then call this method
ForceDefaultBrowserBeaconUpdateForPath() which would make it more reasonable I
think to then force the check and ignore the result.

In all cases I don't really like this weird dependency: how can we test that
this dependency on the side-effect triggered by
GetChromeDefaultStateFromPathAndCacheResult() at least remains healthy?

+}
+
+void UpdateDefaultBrowserBeacon(const base::FilePath& chrome_exe,
+                                BrowserDistribution* distribution,
+                                ShellUtil::DefaultState default_state) {
+  const bool system_install = !InstallUtil::IsPerUserInstall(chrome_exe);
+  const AppRegistrationData& registration_data =
+      distribution->GetAppRegistrationData();
+  switch (default_state) {
+    case ShellUtil::NOT_DEFAULT:
+      Beacon::FirstNotDefault(system_install, registration_data)->Update();
+      break;
+    case ShellUtil::IS_DEFAULT:
+      Beacon::LastWasDefault(system_install, registration_data)->Update();
+      Beacon::FirstNotDefault(system_install, registration_data)->Remove();
+      break;
+    case ShellUtil::UNKNOWN_DEFAULT:
+      break;
+  }
+}
+
+void UpdateOsUpgradeBeacon(bool system_install,
+                           BrowserDistribution* distribution) {
+  Beacon::LastOsUpgrade(system_install, distribution->GetAppRegistrationData())
+      ->Update();
+}
+
+// Beacon ----------------------------------------------------------------------
+
+Beacon::~Beacon() {
+}
+
+// static
+scoped_ptr<Beacon> Beacon::LastOsUpgrade(
+    bool system_install,
+    const AppRegistrationData& registration_data) {
+  return make_scoped_ptr(new Beacon(L"LastOsUpgrade", BeaconType::LAST,
+                                    BeaconScope::PER_INSTALL, system_install,
+                                    registration_data));
+}
+
+// static
+scoped_ptr<Beacon> Beacon::LastWasDefault(
+    bool system_install,
+    const AppRegistrationData& registration_data) {
+  return make_scoped_ptr(new Beacon(L"LastWasDefault", BeaconType::LAST,
+                                    BeaconScope::PER_USER, system_install,
+                                    registration_data));
+}
+
+// static
+scoped_ptr<Beacon> Beacon::FirstNotDefault(
+    bool system_install,
+    const AppRegistrationData& registration_data) {
+  return make_scoped_ptr(new Beacon(L"FirstNotDefault", BeaconType::FIRST,
+                                    BeaconScope::PER_USER, system_install,
+                                    registration_data));
+}
+
+void Beacon::Update() {
+  const REGSAM kAccess = KEY_WOW64_32KEY | KEY_QUERY_VALUE | KEY_SET_VALUE;

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

Haven't seen much installer code since you and Will made those changes, can you
refresh my mind? KEY_WOW64_32KEY means all Chromes (32 or 64) will look at the
32-bit hive?

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

Yes. This is where Google Update keeps its state regardless of the bitness of
the app.

+  base::win::RegKey key(root_, key_path_.c_str(), kAccess);
+
+  // Nothing to update if the key couldn't be created. This should only be the
+  // case for a developer build.
+  if (!key.Valid())
+    return;
+
+  // Nothing to do if this beacon is tracking the first occurrence of an event
+  // that has already been recorded.
+  if (type_ == BeaconType::FIRST && key.HasValue(value_name_.c_str()))
+    return;
+
+  FILETIME now(base::Time::Now().ToFileTime());

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

Do we really need that precise of a timestamp? Feels seconds should be good
enough (i.e. like the install date timestamp in local state)

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

FILETIME is a simple, well-documented platform type. What are the arguments
against it?

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

My argument against it is you need multiple lines of code to break it down when
writing it and to reconstruct it when reading it.

It's also more complex to read (i.e. I can't tell whether this is done correctly
without reading the spec).

So I don't see a reason for using it unless the extra precision (over
Time::(From|To)InternalValue() which gives you a serialized value with
millisecond accuracy) is needed and I don't think it is.

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

Changed to storing using base::Time's internal value as discussed. As it
happens, this is defined in terms of a Windows FILETIME, so it will be easy to
convert the value to a FILETIME in a program that doesn't have access to base/
if needed.

+  ULARGE_INTEGER value;
+  value.u.LowPart = now.dwLowDateTime;
+  value.u.HighPart = now.dwHighDateTime;
+  key.WriteValue(value_name_.c_str(), &value.QuadPart, sizeof(value.QuadPart),
+                 REG_QWORD);
+}
+
+void Beacon::Remove() {
+  const REGSAM kAccess = KEY_WOW64_32KEY | KEY_SET_VALUE;
+  base::win::RegKey key;
+
+  if (key.Open(root_, key_path_.c_str(), kAccess) == ERROR_SUCCESS)

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

Feels weird to use key.Open() here and the RegKey() constructor in Update()
above.

They are logically the same, aren't they? Can we be consistent amongst
implementations in this file?

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

Done.

+    key.DeleteValue(value_name_.c_str());
+}
+
+base::Time Beacon::Get() {
+  const REGSAM kAccess = KEY_WOW64_32KEY | KEY_QUERY_VALUE;
+  base::win::RegKey key;
+  int64_t value;
+
+  if (key.Open(root_, key_path_.c_str(), kAccess) != ERROR_SUCCESS ||
+      key.ReadInt64(value_name_.c_str(), &value) != ERROR_SUCCESS) {
+    return base::Time();
+  }
+
+  ULARGE_INTEGER intermediate;
+  intermediate.QuadPart = static_cast<ULONGLONG>(value);
+  FILETIME beacon_time;
+  beacon_time.dwLowDateTime = intermediate.u.LowPart;
+  beacon_time.dwHighDateTime = intermediate.u.HighPart;
+  return base::Time::FromFileTime(beacon_time);
+}
+
+Beacon::Beacon(base::StringPiece16 name,
+               BeaconType type,
+               BeaconScope scope,
+               bool system_install,
+               const AppRegistrationData& registration_data)
+    : type_(type),
+      root_(system_install ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER),

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

Remind me how ClientStateMedium works again? It's in HKLM but is writeable
without admin rights? (and below we key off the SID to split each user in it? --
shouldn't we instead use HKCU?)

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

Yes.
No. Google Update does not maintain keys in HKCU for per-machine installs.

+      scope_(scope) {
+  Initialize(name, system_install, registration_data);
+}
+
+void Beacon::Initialize(base::StringPiece16 name,
+                        bool system_install,
+                        const AppRegistrationData& registration_data) {
+  // When possible, beacons are located in the app's ClientState key. Per-user
+  // beacons for a per-machine install are located in a beacon-specific sub-key
+  // of the app's ClientStateMedium key.
+  if (!system_install || scope_ == BeaconScope::PER_INSTALL) {
+    key_path_ = registration_data.GetStateKey();
+    value_name_ = name.as_string();
+  } else {
+    key_path_ = registration_data.GetStateMediumKey();
+    key_path_.push_back(L'\\');
+    key_path_.append(name.data(), name.size());
+    // This should never fail. If it does, the beacon will be written in the
+    // key's default value, which is okay since the majority case is likely a
+    // machine with a single user.
+    if (!base::win::GetUserSidString(&value_name_))
+      NOTREACHED();
+  }
+}