Adding metrics on the usefulness of the Win10 version of the Welcome Page

chrome/browser/ui/webui/welcome_win10_handler.cc

Index: chrome/browser/ui/webui/welcome_win10_handler.cc
diff --git a/chrome/browser/ui/webui/welcome_win10_handler.cc b/chrome/browser/ui/webui/welcome_win10_handler.cc
index 9ab794fb6e146025e102abc36417dd4b82f35eb0..21e3954ebe73d81861ba153c685026d26f2b6298 100644
--- a/chrome/browser/ui/webui/welcome_win10_handler.cc
+++ b/chrome/browser/ui/webui/welcome_win10_handler.cc
@@ -5,6 +5,9 @@
 #include "chrome/browser/ui/webui/welcome_win10_handler.h"
 
 #include "base/bind.h"
+#include "base/metrics/histogram.h"
+#include "base/metrics/user_metrics.h"
+#include "base/strings/stringprintf.h"
 #include "base/values.h"
 #include "chrome/browser/shell_integration.h"
 #include "chrome/browser/shell_integration_win.h"
@@ -13,13 +16,54 @@
 #include "content/public/browser/web_contents.h"
 #include "url/gurl.h"
 
-WelcomeWin10Handler::WelcomeWin10Handler() : weak_ptr_factory_(this) {
+namespace {
+
+void RecordDefaultBrowserResult(
+    const std::string& histogram_suffix,
+    shell_integration::DefaultWebClientState default_browser_state) {
+  base::BooleanHistogram::FactoryGet(
+      base::StringPrintf("Welcome.Win10.DefaultPromptResult_%s",
+                         histogram_suffix.c_str()),
+      base::HistogramBase::kUmaTargetedHistogramFlag)
+      ->AddBoolean(default_browser_state == shell_integration::IS_DEFAULT);

[Alexei Svitkine (slow), 2016/11/21 19:17:40]

Instead of this, please use the new functions in histogram_functions.h

[Patrick Monette, 2016/11/21 21:04:22]

TIL. Very useful!

Done.

+}
+
+void RecordPinnedResult(const std::string& histogram_suffix,
+                        bool succeeded,
+                        bool is_pinned) {
+  base::BooleanHistogram::FactoryGet(
+      base::StringPrintf("Welcome.Win10.PinnedPromptResult_%s",
+                         histogram_suffix.c_str()),
+      base::HistogramBase::kUmaTargetedHistogramFlag)
+      ->AddBoolean(is_pinned);
+}
+
+}  // namespace
+
+WelcomeWin10Handler::WelcomeWin10Handler(bool inline_style_variant)
+    : inline_style_variant_(inline_style_variant), weak_ptr_factory_(this) {
   // The check is started as early as possible because waiting for the page to
   // be fully loaded is unnecessarily wasting time.
   StartIsPinnedToTaskbarCheck();
 }
 
-WelcomeWin10Handler::~WelcomeWin10Handler() = default;
+WelcomeWin10Handler::~WelcomeWin10Handler() {
+  std::string histogram_suffix;
+  histogram_suffix += inline_style_variant_ ? "Inline" : "Sectioned";
+  histogram_suffix += pinned_state_result_ ? "Default" : "Combined";
+
+  // Closing the page. Record whether the instructions were useful.
+  (new shell_integration::DefaultBrowserWorker(

[Alexei Svitkine (slow), 2016/11/21 19:17:40]

Nit: I don't think you need the extra outer parens.

[Patrick Monette, 2016/11/21 21:04:22]

Looks weird but it's needed.

.\chrome\browser\ui\webui\welcome_win10_handler.cc(56): error C2143: syntax
error: missing ';' before '->'
.\chrome\browser\ui\webui\welcome_win10_handler.cc(56): error C2059: syntax
error: '->'

+       base::Bind(&RecordDefaultBrowserResult, histogram_suffix)))
+      ->StartCheckIsDefault();
+
+  // The instructions for pinning Chrome to the taskbar are only displayed if
+  // |pinned_state_result_| is true.
+  if (pinned_state_result_) {

[tmartino, 2016/11/21 19:40:08]

1. Why are we checking against pinned_state_result_ instead of
pinned_state_result_.value()? To me it looks like this is switching based on
whether the initial check succeeded, rather than based on the contents of that
check.

2. Can we make it clearer, either through a different name for
pinned_state_result_ or by expanding the comment, that it's the state *prior* to
showing the page that determines whether or not to log this? When skimming, I
initially thought we were only logging successes (e.g. cases when the user did
pin to taskbar) and discarding failures (cases when we show the instructions but
the user doesn't follow them).

[Patrick Monette, 2016/11/21 21:04:22]

Good point. I find that base::Optional<bool> is a tad confusing, but not enough
to refactor it. I removed all usage of the bool operator though.
I added a comment and fixed the logic.

+    shell_integration::win::GetIsPinnedToTaskbarState(
+        base::Closure(), base::Bind(&RecordPinnedResult, histogram_suffix));
+  }
+}
 
 void WelcomeWin10Handler::RegisterMessages() {
   web_ui()->RegisterMessageCallback(
@@ -61,6 +105,8 @@ void WelcomeWin10Handler::HandleGetPinnedToTaskbarState(
 }
 
 void WelcomeWin10Handler::HandleSetDefaultBrowser(const base::ListValue* args) {
+  base::RecordAction(
+      base::UserMetricsAction("Win10WelcomePage_SetAsDefaultBrowser"));
   // The worker owns itself.
   (new shell_integration::DefaultBrowserWorker(
        shell_integration::DefaultWebClientWorkerCallback()))