cros: Add UMA metrics in encryption migration UI.

cros: Add UMA metrics in encryption migration UI.

Some metrics about encryption migration are already recorded in cryptohomed.
In this CL, I'd like to add following UI-related UMA metrics on the app.

Cryptohome.MigrationUI.FirstScreen
Cryptohome.MigrationUI.UserChoice
Cryptohome.MigrationUI.ConsumedBatteryPercent

BUG=714887
TEST=Disable reboot after the migration by modifying the code, and check that chrome://histograms shows the added metrics.

Review-Url: https://codereview.chromium.org/2849833002
Cr-Commit-Position: refs/heads/master@{#468241}
Committed: https://chromium.googlesource.com/chromium/src/+/b75aa7267969141e12f5ece554be20d81d39b044

[fukino, 2017-04-28 12:38:23]

Description was changed from

==========
cros: Add UMA metrics in encryption migration UI.

BUG=714887
==========

to

==========
cros: Add UMA metrics in encryption migration UI.

Some metrics about encryption migration are already recorded in cryptohomed.
In this CL, I'd like to add following UI-related UMA metrics on the app.

Cryptohome.MigrationUI.FirstScreen
Cryptohome.MigrationUI.UserChoice
Cryptohome.MigrationUI.ConsumedBatteryPercent

BUG=714887
TEST=Disable reboot after the migration by modifying the code, and check that
chrome://histograms shows the added metrics.
==========

[fukino, 2017-04-28 12:39:37]

fukino@chromium.org changed reviewers:
+ isherman@chromium.org, xiyuan@chromium.org

[fukino, 2017-04-28 12:39:38]

xiyuan@, could you take a look at the migration UI?
isherman@, could you take a look at histograms.xml?
Thanks!

[xiyuan, 2017-04-28 16:08:56]

lgtm

[Ilya Sherman, 2017-04-28 21:07:52]

Metrics LGTM % nits:

https://codereview.chromium.org/2849833002/diff/1/chrome/browser/ui/webui/chr...
File
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
(right):

https://codereview.chromium.org/2849833002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:60:
enum FirstScreen {
Optional nit: Mebbe use enum class here?

https://codereview.chromium.org/2849833002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:64:
FIRST_SCREEN_MAX
Optional nit: Typically, "MAX" is used as an alias to the last item in the enum;
and "COUNT" is used to count the number of elements in the enum.  So, I'd rename
this to COUNT.

https://codereview.chromium.org/2849833002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:223:
UMA_HISTOGRAM_ENUMERATION(kUmaNameUserChoice, USER_CHOICE_SKIP,
Please create a small wrapper function for recording metrics to this histogram. 
The UMA_HISTOGRAM macros expand to a nontrivial amount of code, so it's nice to
help the compiler avoid duplication of that code by wrapping it up in a single
call-site per histogram.

https://codereview.chromium.org/2849833002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:283:
FIRST_SCREEN_MAX);
For this histogram, you could simply set the metric in the if/else branches, and
then log it at the bottom of this function.

https://codereview.chromium.org/2849833002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2849833002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:9563: +    attempts to log in to the
system and old encryption (eCryptfs) is detedted.
nit: s/detedted/detected

[fukino, 2017-04-29 12:09:51]

The CQ bit was checked by fukino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 12:09:55]

Dry run: 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, 2017-04-29 13:41:03]

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

[commit-bot: I haz the power, 2017-04-29 13:41:08]

Dry run: This issue passed the CQ dry run.

[fukino, 2017-04-29 14:40:31]

Thank you for reviews and suggestions!

https://codereview.chromium.org/2849833002/diff/1/chrome/browser/ui/webui/chr...
File
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
(right):

https://codereview.chromium.org/2849833002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:60:
enum FirstScreen {
On 2017/04/28 21:07:52, Ilya Sherman wrote:
> Optional nit: Mebbe use enum class here?

Done.

https://codereview.chromium.org/2849833002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:64:
FIRST_SCREEN_MAX
On 2017/04/28 21:07:52, Ilya Sherman wrote:
> Optional nit: Typically, "MAX" is used as an alias to the last item in the
enum;
> and "COUNT" is used to count the number of elements in the enum.  So, I'd
rename
> this to COUNT.

Agreed. Done.

https://codereview.chromium.org/2849833002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:223:
UMA_HISTOGRAM_ENUMERATION(kUmaNameUserChoice, USER_CHOICE_SKIP,
On 2017/04/28 21:07:51, Ilya Sherman wrote:
> Please create a small wrapper function for recording metrics to this
histogram. 
> The UMA_HISTOGRAM macros expand to a nontrivial amount of code, so it's nice
to
> help the compiler avoid duplication of that code by wrapping it up in a single
> call-site per histogram.

Done.

https://codereview.chromium.org/2849833002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:283:
FIRST_SCREEN_MAX);
On 2017/04/28 21:07:52, Ilya Sherman wrote:
> For this histogram, you could simply set the metric in the if/else branches,
and
> then log it at the bottom of this function.

I added a wrapper function for consistency with UserChoice.

https://codereview.chromium.org/2849833002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2849833002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:9563: +    attempts to log in to the
system and old encryption (eCryptfs) is detedted.
On 2017/04/28 21:07:52, Ilya Sherman wrote:
> nit: s/detedted/detected

Done.

[fukino, 2017-04-29 14:41:13]

The CQ bit was checked by fukino@chromium.org

[fukino, 2017-04-29 14:41:14]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2849833002/#ps20001
(title: "Address review comments.")

[commit-bot: I haz the power, 2017-04-29 14:41:24]

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, 2017-04-29 15:17:25]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493476873795190,
"parent_rev": "72b93030ac4bc186f52afc120f7dbba84836cc55", "commit_rev":
"b75aa7267969141e12f5ece554be20d81d39b044"}

[commit-bot: I haz the power, 2017-04-29 15:17:36]

Description was changed from

==========
cros: Add UMA metrics in encryption migration UI.

Some metrics about encryption migration are already recorded in cryptohomed.
In this CL, I'd like to add following UI-related UMA metrics on the app.

Cryptohome.MigrationUI.FirstScreen
Cryptohome.MigrationUI.UserChoice
Cryptohome.MigrationUI.ConsumedBatteryPercent

BUG=714887
TEST=Disable reboot after the migration by modifying the code, and check that
chrome://histograms shows the added metrics.
==========

to

==========
cros: Add UMA metrics in encryption migration UI.

Some metrics about encryption migration are already recorded in cryptohomed.
In this CL, I'd like to add following UI-related UMA metrics on the app.

Cryptohome.MigrationUI.FirstScreen
Cryptohome.MigrationUI.UserChoice
Cryptohome.MigrationUI.ConsumedBatteryPercent

BUG=714887
TEST=Disable reboot after the migration by modifying the code, and check that
chrome://histograms shows the added metrics.

Review-Url: https://codereview.chromium.org/2849833002
Cr-Commit-Position: refs/heads/master@{#468241}
Committed:
https://chromium.googlesource.com/chromium/src/+/b75aa7267969141e12f5ece554be...
==========

[commit-bot: I haz the power, 2017-04-29 15:17:37]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/b75aa7267969141e12f5ece554be...

[Ilya Sherman, 2017-05-01 18:02:11]

https://codereview.chromium.org/2849833002/diff/20001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
(right):

https://codereview.chromium.org/2849833002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:89:
static_cast<int>(UserChoice::USER_CHOICE_COUNT));
nit: FYI, static_casts are no longer needed for the UMA_HISTOGRAM_ENUMERATION
macro when working with enum classes.

[fukino, 2017-05-02 04:27:34]

https://codereview.chromium.org/2849833002/diff/20001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
(right):

https://codereview.chromium.org/2849833002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:89:
static_cast<int>(UserChoice::USER_CHOICE_COUNT));
On 2017/05/01 18:02:10, Ilya Sherman wrote:
> nit: FYI, static_casts are no longer needed for the UMA_HISTOGRAM_ENUMERATION
> macro when working with enum classes.

Oh, I did not know it. Thanks!
I'll update it in a follow-up patch.