arc: Skip GMS Sign-In in case Android is already signed-in.

chrome/browser/chromeos/arc/arc_optin_uma.cc

Index: chrome/browser/chromeos/arc/arc_optin_uma.cc
diff --git a/chrome/browser/chromeos/arc/arc_optin_uma.cc b/chrome/browser/chromeos/arc/arc_optin_uma.cc
index b52991a391af47a11c5d99cc826273855cf612c3..ac8b77a7c71b98ea28020bfcafbb12627cc34689 100644
--- a/chrome/browser/chromeos/arc/arc_optin_uma.cc
+++ b/chrome/browser/chromeos/arc/arc_optin_uma.cc
@@ -64,6 +64,13 @@ void UpdateAuthCheckinAttempts(int32_t num_attempts) {
   UMA_HISTOGRAM_SPARSE_SLOWLY("ArcAuth.CheckinAttempts", num_attempts);
 }
 
+void UpdateAuthAccountCheckStatus(mojom::AccountCheckStatus status) {
+  DCHECK(status <= mojom::AccountCheckStatus::CHECK_FAILED);

[Ilya Sherman, 2017/03/27 19:03:28]

nit: DCHECK_LE

[khmel, 2017/03/27 19:29:29]

Done.

+  UMA_HISTOGRAM_ENUMERATION(
+      "ArcAuth.AccountCheckStatus", static_cast<int>(status),
+      static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + 1);

[Ilya Sherman, 2017/03/27 19:03:28]

Hmm, I'm a bit nervous about requiring anyone who edits
components/arc/common/auth.mojom to edit this file as well -- it seems rather
fragile.  Would be great to come up with a design pattern that doesn't require
that -- both for this CL, and more generally to set a precedent for other such
situations.  Does Mojo not validate that enum constants fall in the appropriate
range?  If it does, how does it determine the correct range?

FWIW, another option is to use a sparse histogram, which does not require an
explicit maximum value to be passed to it.

[khmel, 2017/03/27 19:29:29]

IIUC enum in mojom is rather constants (from my experience ARC++ mojom). It
seems sparse histogram is used for sparsely distributed values, as followed from
comments. Probably it is not the best to use it here.

[Ilya Sherman, 2017/03/27 20:07:37]

I'm not following what this means.  Aren't all enums essentially constants?  My
question is: How does Mojo validate deserialized enum values?  Does it simply
not validate them?
I'm not necessarily pushing for it, but I would like to better understand:
What's the disadvantage of using a sparse histogram here?

[khmel, 2017/03/27 20:35:57]

I got feeling that is slower than 'enum' histogram. however I am relying here on
you opinion.

[Ilya Sherman, 2017/03/27 20:54:29]

It is a bit slower, yes.  It's probably not a meaningful performance difference,
though:  UMA_HISTOGRAM_ENUMERATION uses a vector-based implementation, no locks,
caches the histogram pointer in a static local variable. 
UMA_HISTOGRAM_SPARSE_SLOWLY uses a map-based implementation, with locks, and
doesn't cache the pointer.  These differences probably don't matter too much
here -- though perhaps I am mistaken, and this is highly performance-sensitive
code?

[khmel, 2017/03/27 21:01:41]

This code is invoked very infrequently. So sparse histogram should be OK. And
thank you all for your idea! I am totally fine to have it discussed here.

+}
+
 void UpdateSilentAuthCodeUMA(OptInSilentAuthCode state) {
   UMA_HISTOGRAM_ENUMERATION("Arc.OptInSilentAuthCode", static_cast<int>(state),
                             static_cast<int>(OptInSilentAuthCode::SIZE));