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

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

Records an histogram value that indicates if the instructions helped the
user change their default browser and/or pin Chrome to the taskbar.

BUG=648686
Committed: https://crrev.com/444ce88afe130945dae057125d706b789da9282d
Cr-Commit-Position: refs/heads/master@{#434550}

[Patrick Monette, 2016-11-19 00:53:07]

pmonette@chromium.org changed reviewers:
+ tmartino@chromium.org

[Patrick Monette, 2016-11-19 00:53:08]

Hey Tommy. Care to take a first look?

[Patrick Monette, 2016-11-21 18:56:31]

pmonette@chromium.org changed reviewers:
+ asvitkine@chromium.org, michaelpg@chromium.org

[Patrick Monette, 2016-11-21 18:56:32]

Hey!

michaelpg@ Tommy is OOO this week. Can you take a look?
asvitkine@ Take a look at the added metrics.

Note: This patch set depends on another CL.

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

https://codereview.chromium.org/2513953004/diff/1/chrome/browser/ui/webui/wel...
File chrome/browser/ui/webui/welcome_win10_handler.cc (right):

https://codereview.chromium.org/2513953004/diff/1/chrome/browser/ui/webui/wel...
chrome/browser/ui/webui/welcome_win10_handler.cc:28:
->AddBoolean(default_browser_state == shell_integration::IS_DEFAULT);
Instead of this, please use the new functions in histogram_functions.h

https://codereview.chromium.org/2513953004/diff/1/chrome/browser/ui/webui/wel...
chrome/browser/ui/webui/welcome_win10_handler.cc:56: (new
shell_integration::DefaultBrowserWorker(
Nit: I don't think you need the extra outer parens.

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

Will be online intermittently today, offline the rest of the week. If you have a
chance to respond today, I'll take another look, but if not, feel free to submit
without me.

https://codereview.chromium.org/2513953004/diff/1/chrome/browser/ui/webui/wel...
File chrome/browser/ui/webui/welcome_win10_handler.cc (right):

https://codereview.chromium.org/2513953004/diff/1/chrome/browser/ui/webui/wel...
chrome/browser/ui/webui/welcome_win10_handler.cc:62: if (pinned_state_result_) {
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).

https://codereview.chromium.org/2513953004/diff/1/tools/metrics/actions/actio...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2513953004/diff/1/tools/metrics/actions/actio...
tools/metrics/actions/actions.xml:15683: The user clicked "Open Windows
Settings" in the Win10 version of
Do these appear escaped (") in the actual xml, or is it a rendering bug in
Code Review?

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

Thanks for the comments.

https://codereview.chromium.org/2513953004/diff/1/chrome/browser/ui/webui/wel...
File chrome/browser/ui/webui/welcome_win10_handler.cc (right):

https://codereview.chromium.org/2513953004/diff/1/chrome/browser/ui/webui/wel...
chrome/browser/ui/webui/welcome_win10_handler.cc:28:
->AddBoolean(default_browser_state == shell_integration::IS_DEFAULT);
On 2016/11/21 19:17:40, Alexei Svitkine (slow) wrote:
> Instead of this, please use the new functions in histogram_functions.h

TIL. Very useful!

Done.

https://codereview.chromium.org/2513953004/diff/1/chrome/browser/ui/webui/wel...
chrome/browser/ui/webui/welcome_win10_handler.cc:56: (new
shell_integration::DefaultBrowserWorker(
On 2016/11/21 19:17:40, Alexei Svitkine (slow) wrote:
> Nit: I don't think you need the extra outer parens.

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: '->'

https://codereview.chromium.org/2513953004/diff/1/chrome/browser/ui/webui/wel...
chrome/browser/ui/webui/welcome_win10_handler.cc:62: if (pinned_state_result_) {
On 2016/11/21 19:40:08, tmartino wrote:
> 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.
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.
> 
> 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).
I added a comment and fixed the logic.

https://codereview.chromium.org/2513953004/diff/1/tools/metrics/actions/actio...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2513953004/diff/1/tools/metrics/actions/actio...
tools/metrics/actions/actions.xml:15683: The user clicked &quot;Open Windows
Settings&quot; in the Win10 version of
On 2016/11/21 19:40:08, tmartino wrote:
> Do these appear escaped (&quot;) in the actual xml, or is it a rendering bug
in
> Code Review?

Yeah they are really there. Those strings are used on the internal web ui for
UMA and needs the escaped form.

[tmartino, 2016-11-21 21:07:24]

lgtm

[Alexei Svitkine (slow), 2016-11-21 21:12:18]

lgtm

[Patrick Monette, 2016-11-21 21:36:49]

michaelpg@ Take a look as owner please.

[michaelpg, 2016-11-21 22:06:23]

lgtm

[Patrick Monette, 2016-11-21 22:23:40]

Thanks!

[Patrick Monette, 2016-11-21 22:23:45]

The CQ bit was checked by pmonette@chromium.org

[commit-bot: I haz the power, 2016-11-21 22:24:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-21 22:24:17]

This CL has an open dependency (Issue 2512553007 Patch 20001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[Patrick Monette, 2016-11-23 21:55:25]

The CQ bit was checked by pmonette@chromium.org

[Patrick Monette, 2016-11-23 21:55:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
tmartino@chromium.org, asvitkine@chromium.org, michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2513953004/#ps40001
(title: "merge")

[commit-bot: I haz the power, 2016-11-23 21:56:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 23:58:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 23:58:46]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Patrick Monette, 2016-11-24 00:20:44]

The CQ bit was checked by pmonette@chromium.org

[commit-bot: I haz the power, 2016-11-24 00:21:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 02:23:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 02:23:49]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[Patrick Monette, 2016-11-25 18:46:25]

The CQ bit was checked by pmonette@chromium.org

[commit-bot: I haz the power, 2016-11-25 18:46:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-25 21:18:41]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480099585969000,
"parent_rev": "85c640bb29e58f01bf816993890e95e5e179acc2", "commit_rev":
"4d45be4636cca6b56293856c1d39bbfd74f5b0af"}

[commit-bot: I haz the power, 2016-11-25 21:19:06]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-25 21:20:41]

Description was changed from

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

Records an histogram value that indicates if the instructions helped the
user change their default browser and/or pin Chrome to the taskbar.

BUG=648686
==========

to

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

Records an histogram value that indicates if the instructions helped the
user change their default browser and/or pin Chrome to the taskbar.

BUG=648686

Committed: https://crrev.com/444ce88afe130945dae057125d706b789da9282d
Cr-Commit-Position: refs/heads/master@{#434550}
==========

[commit-bot: I haz the power, 2016-11-25 21:20:43]

Patchset 3 (id:??) landed as
https://crrev.com/444ce88afe130945dae057125d706b789da9282d
Cr-Commit-Position: refs/heads/master@{#434550}