UMA to track the reason for re-auth (draft).

UMA to track the reason for re-auth (draft).

BUG=469992

[Ivan Podogov, 2015-04-28 14:27:30]

ginkage@chromium.org changed reviewers:
+ isherman@chromium.org, nkostylev@chromium.org

[Ivan Podogov, 2015-04-28 14:28:15]

PTAL

[Ilya Sherman, 2015-04-28 20:07:18]

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/reauth_stats.h (right):

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/reauth_stats.h:12: // Track all the ways a user
may be sent through the re-auth flow.
Please document that this enum is used to back a histogram, and thus should be
treated as append-only

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/reauth_stats.h:37: OTHER,
nit: It's typically best to list "other" at the beginning of the list.  That
way, if you ever add additional items to the list, "other" remains at one of the
ends of the list, rather than getting buried in the middle somewhere.

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

https://codereview.chromium.org/1114543002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:13767: +    Tracks the reason why a user
was sent through the re-auth flow.
It's not clear to me from this description when the "none" event would be
emitted.  Is that path expected to never be hit?  If not, please update the
summary to clarify when the histogram is recorded.

[Nikita (slow), 2015-04-29 09:21:46]

lgtm

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:579: if
(failure.reason() == AuthFailure::COULD_NOT_MOUNT_CRYPTOHOME)
nit: Add {} even though this is just commented code.

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/reauth_stats.h (right):

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/reauth_stats.h:17: // Password changed, revoked
credentials, account deleted.
nit: insert empty line before comment here and below.

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/reauth_stats.h:18: INVALID_TOKEN_HANDLE,
Set explicit int value for each of these enum constants so that when one of
these is deprecated others that follow it won't change their int values.

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right):

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:352: void
HandleCancelPasswordChangedFlow(const std::string& email);
nit: email -> user_id here and elsewhere in CL

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

https://codereview.chromium.org/1114543002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:13767: +    Tracks the reason why a user
was sent through the re-auth flow.
nit: GAIA re-auth flow

[Ivan Podogov, 2015-04-29 10:34:25]

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:579: if
(failure.reason() == AuthFailure::COULD_NOT_MOUNT_CRYPTOHOME)
On 2015/04/29 09:21:46, Nikita Kostylev wrote:
> nit: Add {} even though this is just commented code.

Done.

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/reauth_stats.h (right):

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/reauth_stats.h:12: // Track all the ways a user
may be sent through the re-auth flow.
On 2015/04/28 20:07:18, Ilya Sherman wrote:
> Please document that this enum is used to back a histogram, and thus should be
> treated as append-only

Done.

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/reauth_stats.h:17: // Password changed, revoked
credentials, account deleted.
On 2015/04/29 09:21:46, Nikita Kostylev wrote:
> nit: insert empty line before comment here and below.

Done.

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/reauth_stats.h:18: INVALID_TOKEN_HANDLE,
On 2015/04/29 09:21:46, Nikita Kostylev wrote:
> Set explicit int value for each of these enum constants so that when one of
> these is deprecated others that follow it won't change their int values.

Done.

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/reauth_stats.h:37: OTHER,
On 2015/04/28 20:07:18, Ilya Sherman wrote:
> nit: It's typically best to list "other" at the beginning of the list.  That
> way, if you ever add additional items to the list, "other" remains at one of
the
> ends of the list, rather than getting buried in the middle somewhere.

Done.

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right):

https://codereview.chromium.org/1114543002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:352: void
HandleCancelPasswordChangedFlow(const std::string& email);
On 2015/04/29 09:21:46, Nikita Kostylev wrote:
> nit: email -> user_id here and elsewhere in CL

Done.

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

https://codereview.chromium.org/1114543002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:13767: +    Tracks the reason why a user
was sent through the re-auth flow.
On 2015/04/29 09:21:46, Nikita Kostylev wrote:
> nit: GAIA re-auth flow

Done.

https://codereview.chromium.org/1114543002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:13767: +    Tracks the reason why a user
was sent through the re-auth flow.
On 2015/04/28 20:07:18, Ilya Sherman wrote:
> It's not clear to me from this description when the "none" event would be
> emitted.  Is that path expected to never be hit?  If not, please update the
> summary to clarify when the histogram is recorded.

This should not be hit, this value is used to indicate that we have nothing to
emit.

[Ilya Sherman, 2015-04-30 05:00:37]

LGTM

[Ivan Podogov, 2015-05-01 09:12:52]

The CQ bit was checked by ginkage@chromium.org

[Ivan Podogov, 2015-05-01 09:12:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
nkostylev@chromium.org
Link to the patchset: https://codereview.chromium.org/1114543002/#ps40001
(title: "Code review fixes.")

[commit-bot: I haz the power, 2015-05-01 09:13:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1114543002/40001

[commit-bot: I haz the power, 2015-05-01 10:06:18]

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

[commit-bot: I haz the power, 2015-05-01 10:06:19]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nikita (slow), 2015-05-07 10:35:45]

Landing in https://codereview.chromium.org/1132523002/#