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

chrome/browser/ui/webui/welcome_win10_handler.cc

 // Copyright 2016 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/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"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/browser_thread.h"
 #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(
       "handleSetDefaultBrowser",
       base::Bind(&WelcomeWin10Handler::HandleSetDefaultBrowser,
                  base::Unretained(this)));
   web_ui()->RegisterMessageCallback(
       "handleContinue",
       base::Bind(&WelcomeWin10Handler::HandleContinue, base::Unretained(this)));
   web_ui()->RegisterMessageCallback(
@@ skipping 21 matching lines @@
   // Only wait for a small amount of time for the result. If the timer fires,
   // it will be assumed that Chrome is pinned to the taskbar.
   constexpr base::TimeDelta kPinnedToTaskbarTimeout =
       base::TimeDelta::FromMilliseconds(200);
   timer_.Start(FROM_HERE, kPinnedToTaskbarTimeout,
                base::Bind(&WelcomeWin10Handler::OnIsPinnedToTaskbarDetermined,
                           base::Unretained(this), true));
 }
 
 void WelcomeWin10Handler::HandleSetDefaultBrowser(const base::ListValue* args) {
+  base::RecordAction(
+      base::UserMetricsAction("Win10WelcomePage_SetAsDefaultBrowser"));
   // The worker owns itself.
   (new shell_integration::DefaultBrowserWorker(
        shell_integration::DefaultWebClientWorkerCallback()))
       ->StartSetAsDefault();
 }
 
 void WelcomeWin10Handler::HandleContinue(const base::ListValue* args) {
   web_ui()->GetWebContents()->GetController().LoadURL(
       GURL(chrome::kChromeUINewTabURL), content::Referrer(),
       ui::PageTransition::PAGE_TRANSITION_LINK, std::string());
@@ skipping 33 matching lines @@
   // sent back.
   if (!pinned_state_callback_id_.empty())
     SendPinnedToTaskbarStateResult();
 }
 
 void WelcomeWin10Handler::SendPinnedToTaskbarStateResult() {
   ResolveJavascriptCallback(
       base::StringValue(pinned_state_callback_id_),
       base::FundamentalValue(pinned_state_result_.value()));
 }