Use a Finch Experiment to control the Browser Blacklist

chrome/browser/chrome_elf_init_win.cc

+// Copyright 2013 The Chromium Authors. All rights reserved.

[robertshield, 2014/01/02 20:42:22]

2014

[csharp, 2014/01/02 21:57:59]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/metrics/field_trial.h"
+#include "base/metrics/histogram.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/win/registry.h"
+#include "chrome/browser/chrome_elf_init_win.h"
+#include "chrome/common/chrome_version_info.h"
+#include "chrome_elf/blacklist/blacklist.h"
+
+namespace {
+
+const char kBrowserBlacklistTrialName[] = "BrowserBlacklist";
+const char kBrowserBlacklistTrialEnabledGroupName[] = "Enabled";

[robertshield, 2014/01/02 20:42:22]

nit: alphabetize these

[csharp, 2014/01/02 21:57:59]

I looked at a couple files and this order (trial name followed by group names)
seems to be the general pattern. 

Although maybe jwd@ or isherman@ can shed more light on this.

[robertshield, 2014/01/03 03:21:42]

Ok, not a strong preference. If this is conventional I'm fine with it. 

+
+// This enum is used to define the buckets for an enumerated UMA histogram.
+// Hence,
+//   (a) existing enumerated constants should never be deleted or reordered, and
+//   (b) new constants should only be appended in front of
+//       BLACKLIST_SETUP_EVENT_MAX.
+enum BlacklistSetupEventType {
+  // The blacklist beacon has placed to enable the browser blacklisting.
+  BLACKLIST_SETUP_ENABLED = 0,
+
+  // The blacklist was successfully enabled.
+  BLACKLIST_SETUP_RAN_SUCCESSFULLY,
+
+  // The blacklist setup code failed to execute.
+  BLACKLIST_SETUP_FAILED,
+
+  // Always keep this at the end.
+  BLACKLIST_SETUP_EVENT_MAX,
+};
+
+void RecordBlacklistSetupEvent(BlacklistSetupEventType blacklist_setup_event) {
+  UMA_HISTOGRAM_ENUMERATION("Blacklist.Setup",
+                            blacklist_setup_event,
+                            BLACKLIST_SETUP_EVENT_MAX);
+}
+
+}  // namespace
+
+void SetupChromeElfInitialization() {
+  if (base::FieldTrialList::FindFullName(kBrowserBlacklistTrialName) ==
+      kBrowserBlacklistTrialEnabledGroupName) {
+    BrowserBlacklistBeaconSetup();
+  } else {
+    // Disable the blacklist for all future runs by removing the beacon.
+    base::win::RegKey blacklist_registry_key(HKEY_CURRENT_USER);
+    blacklist_registry_key.DeleteKey(blacklist::kRegistryBeaconPath);
+  }
+}
+
+void BrowserBlacklistBeaconSetup() {
+  base::win::RegKey blacklist_registry_key(HKEY_CURRENT_USER,
+                                           blacklist::kRegistryBeaconPath,
+                                           KEY_READ | KEY_WRITE);

[robertshield, 2014/01/02 20:42:22]

I believe this should be KEY_QUERY_VALUE | KEY_SET_VALUE.

[csharp, 2014/01/02 21:57:59]

Yup, that seems to work.

+
+  DWORD blacklist_state = 0;
+  blacklist_registry_key.ReadValueDW(blacklist::kBeaconState, &blacklist_state);
+
+  // Record the results of the latest blacklist setup.
+  if (blacklist_state == blacklist::BLACKLIST_ENABLED) {
+    RecordBlacklistSetupEvent(BLACKLIST_SETUP_RAN_SUCCESSFULLY);
+  } else if (blacklist_state == blacklist::BLACKLIST_SETUP_RUNNING) {
+    RecordBlacklistSetupEvent(BLACKLIST_SETUP_FAILED);

[robertshield, 2014/01/02 20:42:22]

as discussed, can we check the version here to make sure we don't misreport it
across an upgrade?

[csharp, 2014/01/02 21:57:59]

Done.

+
+    // Since the setup failed, mark the blacklist as disabled so we don't
+    // try it again for this version.
+    blacklist_registry_key.WriteValue(blacklist::kBeaconState,
+                                      blacklist::BLACKLIST_DISABLED);
+  }
+
+  // Find the version the blacklist was last enabled for.
+  std::wstring blacklist_version(L"");
+  blacklist_registry_key.ReadValue(blacklist::kBeaconVersion,
+                                   &blacklist_version);
+
+  // If the blacklist hasn't been enabled for this version yet, enable it.
+  chrome::VersionInfo version_info;
+  base::string16 current_version = base::UTF8ToUTF16(version_info.Version());

[robertshield, 2014/01/02 20:42:22]

nit: prefer direct construction vs assignment (it makes it clearer that a
constructor is being called):
base::string16 current_version(base::UTF8ToUTF16(version_info.Version()));

[csharp, 2014/01/02 21:57:59]

Done.

+  if (blacklist_version != current_version) {
+    blacklist_registry_key.WriteValue(blacklist::kBeaconVersion,
+                                      current_version.c_str());
+
+    blacklist_registry_key.WriteValue(blacklist::kBeaconState,
+                                      blacklist::BLACKLIST_ENABLED);
+
+    RecordBlacklistSetupEvent(BLACKLIST_SETUP_ENABLED);
+  }
+}