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

chrome/browser/chromeos/arc/arc_optin_uma.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/arc/arc_optin_uma.h"
 
 #include <string>
 
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
@@ skipping 46 matching lines @@
   base::UmaHistogramCustomTimes(histogram_name, elapsed_time,
                                 base::TimeDelta::FromSeconds(1) /* minimum */,
                                 base::TimeDelta::FromMinutes(3) /* maximum */,
                                 50 /* bucket_count */);
 }
 
 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));
 }
 
 std::ostream& operator<<(std::ostream& os, const ProvisioningResult& result) {
 #define MAP_PROVISIONING_RESULT(name) \
   case ProvisioningResult::name:      \
     return os << #name
 
@@ skipping 24 matching lines @@
 
 #undef MAP_PROVISIONING_RESULT
 
   // Some compilers report an error even if all values of an enum-class are
   // covered exhaustively in a switch statement.
   NOTREACHED() << "Invalid value " << static_cast<int>(result);
   return os;
 }
 
 }  // namespace arc