Handles unknown signin status and collect more stats.

Handles unknown signin status and collect more stats.

Considers unknown signin status as a valid status when browser is added (For mac, it's possible to have chrome running with no browser window open, which should be signin-status-unknown).

Collects more stats when getting signin status. (record it in histograms whenever any error is seen.)

BUG=412794
Committed: https://crrev.com/c24e097ef7cd87b03e8090898cf7fc4cf69225e2
Cr-Commit-Position: refs/heads/master@{#294192}

[yao, 2014-09-08 21:21:35]

Patchset #1 (id:1) has been deleted

[yao, 2014-09-08 21:27:08]

Patchset #2 (id:40001) has been deleted

[yao, 2014-09-08 21:27:18]

yiyaoliu@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-09-09 15:07:31]

https://codereview.chromium.org/554863002/diff/20001/chrome/browser/metrics/s...
File chrome/browser/metrics/signin_status_metrics_provider.cc (right):

https://codereview.chromium.org/554863002/diff/20001/chrome/browser/metrics/s...
chrome/browser/metrics/signin_status_metrics_provider.cc:227: if
(profile_list.size() == 0) {
Nit: .empty()

https://codereview.chromium.org/554863002/diff/20001/chrome/browser/metrics/s...
chrome/browser/metrics/signin_status_metrics_provider.cc:231: } else if
(profile_list.size() > 0 && opened_profiles_count == 0) {
The profile_list.size() > 0 shouldn't be necessary, since the if above checks
it's not empty (and negative is not possible).

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

https://codereview.chromium.org/554863002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:38747: +  <int value="1" label="Error:
No profiles found."/>
Please don't renumber existing entries. Put the new one at the end.

[yao, 2014-09-09 16:39:52]

Added 2 more enums to "UMA.ComputeCurrentSigninStatus". 

I think it should be recorded every time signin_status_ is set to status_error,
but also don't feel like adding more histograms. Maybe we can rename this
histograms if you find it confusing. WDYT?

https://codereview.chromium.org/554863002/diff/20001/chrome/browser/metrics/s...
File chrome/browser/metrics/signin_status_metrics_provider.cc (right):

https://codereview.chromium.org/554863002/diff/20001/chrome/browser/metrics/s...
chrome/browser/metrics/signin_status_metrics_provider.cc:227: if
(profile_list.size() == 0) {
On 2014/09/09 15:07:31, Alexei Svitkine wrote:
> Nit: .empty()

Done.

https://codereview.chromium.org/554863002/diff/20001/chrome/browser/metrics/s...
chrome/browser/metrics/signin_status_metrics_provider.cc:231: } else if
(profile_list.size() > 0 && opened_profiles_count == 0) {
On 2014/09/09 15:07:31, Alexei Svitkine wrote:
> The profile_list.size() > 0 shouldn't be necessary, since the if above checks
> it's not empty (and negative is not possible).

Done.

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

https://codereview.chromium.org/554863002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:38747: +  <int value="1" label="Error:
No profiles found."/>
On 2014/09/09 15:07:31, Alexei Svitkine wrote:
> Please don't renumber existing entries. Put the new one at the end.

I actually didn't remember it, just updated the label, but they represent the
same thing.

[Alexei Svitkine (slow), 2014-09-09 19:17:45]

https://codereview.chromium.org/554863002/diff/60001/chrome/browser/metrics/s...
File chrome/browser/metrics/signin_status_metrics_provider.cc (right):

https://codereview.chromium.org/554863002/diff/60001/chrome/browser/metrics/s...
chrome/browser/metrics/signin_status_metrics_provider.cc:201: } else if
(signin_status_ == UNKNOWN_SIGNIN_STATUS && signed_in) {
Nit: Combine the ifs and use a ?:

} else if (signin_status_ == UNKNOWN_SIGNIN_STATUS) {
  signin_status = (signed_in ? ALL_PROFILES_SIGNED_IN :
                               ALL_PROFILES_NOT_SIGNED_IN);
}

But also please add a comment in the if explaining a case where this could
happen so it's clear what case this is handling.

[yao, 2014-09-09 19:37:32]

https://codereview.chromium.org/554863002/diff/60001/chrome/browser/metrics/s...
File chrome/browser/metrics/signin_status_metrics_provider.cc (right):

https://codereview.chromium.org/554863002/diff/60001/chrome/browser/metrics/s...
chrome/browser/metrics/signin_status_metrics_provider.cc:201: } else if
(signin_status_ == UNKNOWN_SIGNIN_STATUS && signed_in) {
On 2014/09/09 19:17:45, Alexei Svitkine wrote:
> Nit: Combine the ifs and use a ?:
> 
> } else if (signin_status_ == UNKNOWN_SIGNIN_STATUS) {
>   signin_status = (signed_in ? ALL_PROFILES_SIGNED_IN :
>                                ALL_PROFILES_NOT_SIGNED_IN);
> }
> 
> But also please add a comment in the if explaining a case where this could
> happen so it's clear what case this is handling.

Done.

[Alexei Svitkine (slow), 2014-09-09 20:07:02]

lgtm, but please file a crbug for this work

[yao, 2014-09-10 15:45:31]

The CQ bit was checked by yiyaoliu@chromium.org

[commit-bot: I haz the power, 2014-09-10 15:46:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yiyaoliu@chromium.org/554863002/80001

[commit-bot: I haz the power, 2014-09-10 16:40:52]

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

[commit-bot: I haz the power, 2014-09-10 16:40:53]

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

[yao, 2014-09-10 17:00:05]

The CQ bit was checked by yiyaoliu@chromium.org

[commit-bot: I haz the power, 2014-09-10 17:01:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yiyaoliu@chromium.org/554863002/100001

[commit-bot: I haz the power, 2014-09-10 18:10:46]

Committed patchset #4 (id:100001) as feb8b52c19c739bc593c084fa237a2211397decd

[commit-bot: I haz the power, 2014-09-10 18:14:13]

Patchset 4 (id:??) landed as
https://crrev.com/c24e097ef7cd87b03e8090898cf7fc4cf69225e2
Cr-Commit-Position: refs/heads/master@{#294192}