Added UMA metrics for ChromeOS first-run UI.

Added UMA metrics for ChromeOS first-run UI.

Added following metrics:
 * CrosFirstRun.DialogShown
 * CrosFirstRun.FurthestStep
 * CrosFirstRun.TimeSpent
 * CrosFirstRun.TimeSpentOnStepAppList
 * CrosFirstRun.TimeSpentOnStepHelp
 * CrosFirstRun.TimeSpentOnStepTray
 * CrosFirstRun.TutorialCompletion
 * CrosFirstRun.TutorialLaunched

BUG=269291
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245212

[dzhioev (left Google), 2014-01-13 15:01:18]

Hi, please review
ygorshenin@: chrome/browser/chromeos/first_run/*
asvitkine@: histograms.xml
finnur@: first_run_private_api.cc

[Finnur, 2014-01-13 15:42:16]

first_run_private_api.cc lgtm

[dzhioev (left Google), 2014-01-14 14:31:23]

Forgot to add reviewers

[ygorshenin1, 2014-01-15 09:34:24]

https://codereview.chromium.org/131023003/diff/1/chrome/browser/chromeos/firs...
File chrome/browser/chromeos/first_run/first_run_controller.cc (right):

https://codereview.chromium.org/131023003/diff/1/chrome/browser/chromeos/firs...
chrome/browser/chromeos/first_run/first_run_controller.cc:69:
start_time_(base::Time::Now()) {}
Please, move initialization of start_time_ to Init().

https://codereview.chromium.org/131023003/diff/1/chrome/browser/chromeos/firs...
chrome/browser/chromeos/first_run/first_run_controller.cc:170:
GetCurrentStep()->Show();
Could you please transform this code in this way:
if (GetCurrentStep()) {
  ...
  return;
}
...

[dzhioev (left Google), 2014-01-15 13:50:59]

https://codereview.chromium.org/131023003/diff/1/chrome/browser/chromeos/firs...
File chrome/browser/chromeos/first_run/first_run_controller.cc (right):

https://codereview.chromium.org/131023003/diff/1/chrome/browser/chromeos/firs...
chrome/browser/chromeos/first_run/first_run_controller.cc:69:
start_time_(base::Time::Now()) {}
On 2014/01/15 09:34:25, ygorshenin1 wrote:
> Please, move initialization of start_time_ to Init().

Done.

https://codereview.chromium.org/131023003/diff/1/chrome/browser/chromeos/firs...
chrome/browser/chromeos/first_run/first_run_controller.cc:170:
GetCurrentStep()->Show();
On 2014/01/15 09:34:25, ygorshenin1 wrote:
> Could you please transform this code in this way:
> if (GetCurrentStep()) {
>   ...
>   return;
> }
> ...

Done.

[Alexei Svitkine (slow), 2014-01-15 16:02:37]

https://codereview.chromium.org/131023003/diff/1/chrome/browser/chromeos/firs...
File chrome/browser/chromeos/first_run/steps/tray_step.h (right):

https://codereview.chromium.org/131023003/diff/1/chrome/browser/chromeos/firs...
chrome/browser/chromeos/first_run/steps/tray_step.h:20: virtual void DoShow()
OVERRIDE;
Nit: DISALLOW_COPY_AND_ASSIGN()?

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/first_run.cc (right):

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/first_run.cc:105:
UMA_HISTOGRAM_COUNTS("CrosFirstRun.TutorialLaunched", 1);
This should probably be UMA_HISTOGRAM_BOOLEAN or alternatively, it should be
grouped with some other enum histograms, to make things easier to compare.

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/first_run_controller.cc (right):

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/first_run_controller.cc:97:
"CrosFirstRun.FurthestStep", furthest_step, steps_.size());
Nit: Can you wrap this consistently with the next histogram? (Whichever way is
fine, just make them both use the same style of wrapping.)

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/metrics.h (right):

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/metrics.h:13: kTutorialNotFinished,
I believe enums should use FOO_BAR style in Chromium style.

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/step.cc (right):

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/step.cc:74: base::HistogramBase* histogram =
base::Histogram::FactoryTimeGet(
Nit: Maybe add a comment with an UMA macro declaration that this expanded form
corresponds to.

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/steps/app_list_step.h (right):

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/steps/app_list_step.h:21: };
Nit: DISALLOW_COPY_AND_ASSIGN()?

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/steps/help_step.cc (right):

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/steps/help_step.cc:34: void
HelpStep::DoOnAfterHide() { shell_helper()->CloseTrayBubble(); }
Nit: I think putting method on the same line is done only for hacker_case
trivial methods declared with the class declaration.

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/steps/tray_step.h (right):

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/steps/tray_step.h:21: };
Can this have DISALLOW_COPY_AND_ASSIGN()?

https://codereview.chromium.org/131023003/diff/120001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/131023003/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1727: +<histogram
name="CrosFirstRun.FurthestStep">
Can you add an enum for the steps?

https://codereview.chromium.org/131023003/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1735: +<histogram
name="CrosFirstRun.TimeSpentOnAppListStep" units="ms">
For all the CrosFirstRun.TimeSpentOn* histograms, can you use a <fieldtrial>
block to create common-suffixed histograms (see examples in this file and some
info at the top of the file on how to do this and), to avoid repeating these?
Then, you can add "CrosFirstRun.TimeSpentOn" as an <affected-histogram>. (Note
that you can specify a custom separator string on the fieldtrial block).

[dzhioev (left Google), 2014-01-15 22:12:54]

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/first_run.cc (right):

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/first_run.cc:105:
UMA_HISTOGRAM_COUNTS("CrosFirstRun.TutorialLaunched", 1);
On 2014/01/15 16:02:38, Alexei Svitkine wrote:
> This should probably be UMA_HISTOGRAM_BOOLEAN or alternatively, it should be
> grouped with some other enum histograms, to make things easier to compare.

It's intended to count how many times tutorial was launched. Formally it is
histogram with only one value - 1. I didn't find other way to count events
except for RecordAction("SOMETHING"). But stats for events recorded by
RecordAction are saved only for 7 days.

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/first_run_controller.cc (right):

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/first_run_controller.cc:97:
"CrosFirstRun.FurthestStep", furthest_step, steps_.size());
On 2014/01/15 16:02:38, Alexei Svitkine wrote:
> Nit: Can you wrap this consistently with the next histogram? (Whichever way is
> fine, just make them both use the same style of wrapping.)

Done.

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/metrics.h (right):

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/metrics.h:13: kTutorialNotFinished,
On 2014/01/15 16:02:38, Alexei Svitkine wrote:
> I believe enums should use FOO_BAR style in Chromium style.

Both styles are allowed, furthermore Google Style Guide says that "Preferably,
the individual enumerators should be named like constants." (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer...
).
Also kFooBar style is widely used in Chromium sources.

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/step.cc (right):

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/step.cc:74: base::HistogramBase* histogram =
base::Histogram::FactoryTimeGet(
On 2014/01/15 16:02:38, Alexei Svitkine wrote:
> Nit: Maybe add a comment with an UMA macro declaration that this expanded form
> corresponds to.

Done. Added comment.

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/steps/app_list_step.h (right):

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/steps/app_list_step.h:21: };
On 2014/01/15 16:02:38, Alexei Svitkine wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN()?

Done.

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/steps/help_step.cc (right):

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/steps/help_step.cc:34: void
HelpStep::DoOnAfterHide() { shell_helper()->CloseTrayBubble(); }
On 2014/01/15 16:02:38, Alexei Svitkine wrote:
> Nit: I think putting method on the same line is done only for hacker_case
> trivial methods declared with the class declaration. 
That's what 'git cl format' did and I missed it.
Returned formatting as it was before.

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/steps/tray_step.h (right):

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/steps/tray_step.h:21: };
On 2014/01/15 16:02:38, Alexei Svitkine wrote:
> Can this have DISALLOW_COPY_AND_ASSIGN()?

Done.

https://codereview.chromium.org/131023003/diff/120001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/131023003/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1727: +<histogram
name="CrosFirstRun.FurthestStep">
On 2014/01/15 16:02:38, Alexei Svitkine wrote:
> Can you add an enum for the steps?
We want to know how many steps user saw rather than which step was last. Order
of steps could change eventually, new steps could appear. That's why we can't
use enum here.

https://codereview.chromium.org/131023003/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1735: +<histogram
name="CrosFirstRun.TimeSpentOnAppListStep" units="ms">
On 2014/01/15 16:02:38, Alexei Svitkine wrote:
> For all the CrosFirstRun.TimeSpentOn* histograms, can you use a <fieldtrial>
> block to create common-suffixed histograms (see examples in this file and some
> info at the top of the file on how to do this and), to avoid repeating these?
> Then, you can add "CrosFirstRun.TimeSpentOn" as an <affected-histogram>. (Note
> that you can specify a custom separator string on the fieldtrial block).

Done.

[Alexei Svitkine (slow), 2014-01-15 22:21:36]

LGTM % my comments below

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/first_run.cc (right):

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/first_run.cc:105:
UMA_HISTOGRAM_COUNTS("CrosFirstRun.TutorialLaunched", 1);
On 2014/01/15 22:12:54, dzhioev wrote:
> On 2014/01/15 16:02:38, Alexei Svitkine wrote:
> > This should probably be UMA_HISTOGRAM_BOOLEAN or alternatively, it should be
> > grouped with some other enum histograms, to make things easier to compare.
> 
> It's intended to count how many times tutorial was launched. Formally it is
> histogram with only one value - 1. I didn't find other way to count events
> except for RecordAction("SOMETHING"). But stats for events recorded by
> RecordAction are saved only for 7 days. 

UMA_HISTOGRAM_BOOLEAN() should do what you want here - it will sum up the number
of times its hit in the dashboards. In fact, it should be equivalent (but more
efficient at runtime in memory use).

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/metrics.h (right):

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/metrics.h:13: kTutorialNotFinished,
On 2014/01/15 22:12:54, dzhioev wrote:
> On 2014/01/15 16:02:38, Alexei Svitkine wrote:
> > I believe enums should use FOO_BAR style in Chromium style.
> 
> Both styles are allowed, furthermore Google Style Guide says that "Preferably,
> the individual enumerators should be named like constants." (
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer...
> ).
> Also kFooBar style is widely used in Chromium sources.

Chromiums style overrides the Google one in this case: "Though the Google C++
Style Guide now says to use kConstantNaming for enums, Chromium was written
using MACRO_STYLE naming.  Continue to use this style for consistency."

However, if you say that ChromeOS already uses kStyle - then I won't press it
here (since the argument is "for consistency").

https://codereview.chromium.org/131023003/diff/120001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/131023003/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1727: +<histogram
name="CrosFirstRun.FurthestStep">
On 2014/01/15 22:12:54, dzhioev wrote:
> On 2014/01/15 16:02:38, Alexei Svitkine wrote:
> > Can you add an enum for the steps?
> We want to know how many steps user saw rather than which step was last. Order
> of steps could change eventually, new steps could appear. That's why we can't
> use enum here.

Okay, please expand the description here to capture this motivation.

[dzhioev (left Google), 2014-01-16 13:21:14]

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/first_run.cc (right):

https://codereview.chromium.org/131023003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/first_run.cc:105:
UMA_HISTOGRAM_COUNTS("CrosFirstRun.TutorialLaunched", 1);
On 2014/01/15 22:21:36, Alexei Svitkine wrote:
> On 2014/01/15 22:12:54, dzhioev wrote:
> > On 2014/01/15 16:02:38, Alexei Svitkine wrote:
> > > This should probably be UMA_HISTOGRAM_BOOLEAN or alternatively, it should
be
> > > grouped with some other enum histograms, to make things easier to compare.
> > 
> > It's intended to count how many times tutorial was launched. Formally it is
> > histogram with only one value - 1. I didn't find other way to count events
> > except for RecordAction("SOMETHING"). But stats for events recorded by
> > RecordAction are saved only for 7 days. 
> 
> UMA_HISTOGRAM_BOOLEAN() should do what you want here - it will sum up the
number
> of times its hit in the dashboards. In fact, it should be equivalent (but more
> efficient at runtime in memory use).

Done.

https://codereview.chromium.org/131023003/diff/120001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/131023003/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1727: +<histogram
name="CrosFirstRun.FurthestStep">
On 2014/01/15 22:21:36, Alexei Svitkine wrote:
> On 2014/01/15 22:12:54, dzhioev wrote:
> > On 2014/01/15 16:02:38, Alexei Svitkine wrote:
> > > Can you add an enum for the steps?
> > We want to know how many steps user saw rather than which step was last.
Order
> > of steps could change eventually, new steps could appear. That's why we
can't
> > use enum here.
> 
> Okay, please expand the description here to capture this motivation.

Done.

[ygorshenin1, 2014-01-16 13:27:25]

lgtm

[commit-bot: I haz the power, 2014-01-16 14:35:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dzhioev@chromium.org/131023003/300001

[commit-bot: I haz the power, 2014-01-16 16:14:01]

Change committed as 245212