Handles unknown signin status and collect more stats.

chrome/browser/metrics/signin_status_metrics_provider.cc

 // Copyright 2014 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/metrics/signin_status_metrics_provider.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 10 matching lines @@
 #if !defined(OS_ANDROID)
 #include "chrome/browser/ui/browser_finder.h"
 #endif
 
 namespace {
 
 // The event of calling function ComputeCurrentSigninStatus and the errors
 // occurred during the function execution.
 enum ComputeSigninStatus {
   ENTERED_COMPUTE_SIGNIN_STATUS,
-  ERROR_COMPUTE_SIGNIN_STATUS,
+  ERROR_NO_PROFILE_FOUND,
+  NO_BROWSER_OPENED,
+  USER_SIGNIN_WHEN_STATUS_UNKNOWN,
+  USER_SIGNOUT_WHEN_STATUS_UNKNOWN,
   COMPUTE_SIGNIN_STATUS_MAX,
 };
 
 void RecordComputeSigninStatusHistogram(ComputeSigninStatus status) {
   UMA_HISTOGRAM_ENUMERATION("UMA.ComputeCurrentSigninStatus", status,
                             COMPUTE_SIGNIN_STATUS_MAX);
 }
 
 }  // namespace
 
@@ skipping 74 matching lines @@
 void SigninStatusMetricsProvider::SigninManagerShutdown(
     SigninManagerBase* manager) {
   if (scoped_observer_.IsObserving(manager))
     scoped_observer_.Remove(manager);
 }
 
 void SigninStatusMetricsProvider::GoogleSigninSucceeded(
     const std::string& account_id,
     const std::string& username,
     const std::string& password) {
-  if (signin_status_ == ALL_PROFILES_NOT_SIGNED_IN)
+  if (signin_status_ == ALL_PROFILES_NOT_SIGNED_IN) {
     signin_status_ = MIXED_SIGNIN_STATUS;
+  } else if (signin_status_ == UNKNOWN_SIGNIN_STATUS) {
+    // There should have at least one browser opened if the user can sign in, so
+    // signin_status_ value should not be unknown.
+    signin_status_ = ERROR_GETTING_SIGNIN_STATUS;
+    RecordComputeSigninStatusHistogram(USER_SIGNIN_WHEN_STATUS_UNKNOWN);
+  }
 }
 
 void SigninStatusMetricsProvider::GoogleSignedOut(const std::string& account_id,
                                                   const std::string& username) {
-  if (signin_status_ == ALL_PROFILES_SIGNED_IN)
+  if (signin_status_ == ALL_PROFILES_SIGNED_IN) {
     signin_status_ = MIXED_SIGNIN_STATUS;
+  } else if (signin_status_ == UNKNOWN_SIGNIN_STATUS) {
+    // There should have at least one browser opened if the user can sign out,
+    // so signin_status_ value should not be unknown.
+    signin_status_ = ERROR_GETTING_SIGNIN_STATUS;
+    RecordComputeSigninStatusHistogram(USER_SIGNOUT_WHEN_STATUS_UNKNOWN);
+  }
 }
 
 void SigninStatusMetricsProvider::Initialize() {
   // Add observers.
 #if !defined(OS_ANDROID)
   // On Android, there is always only one profile in any situation, opening new
   // windows (which is possible with only some Android devices) will not change
   // the opened profiles signin status.
   BrowserList::AddObserver(this);
 #endif
@@ skipping 17 matching lines @@
   if (profiles.empty()) {
     signin_status_ = UNKNOWN_SIGNIN_STATUS;
   } else {
     ComputeCurrentSigninStatus();
   }
 }
 
 void SigninStatusMetricsProvider::UpdateInitialSigninStatus(
     size_t total_count,
     size_t signed_in_profiles_count) {
-  RecordComputeSigninStatusHistogram(ENTERED_COMPUTE_SIGNIN_STATUS);
-
-  if (total_count == 0) {
-    // This should never happen. If it does, record it in histogram.
-    RecordComputeSigninStatusHistogram(ERROR_COMPUTE_SIGNIN_STATUS);
-    signin_status_ = UNKNOWN_SIGNIN_STATUS;
-  } else if (signed_in_profiles_count == 0) {
+  // total_count is known to be bigger than 0.
+  if (signed_in_profiles_count == 0) {
     signin_status_ = ALL_PROFILES_NOT_SIGNED_IN;
   } else if (total_count == signed_in_profiles_count) {
     signin_status_ = ALL_PROFILES_SIGNED_IN;
   } else {
     signin_status_ = MIXED_SIGNIN_STATUS;
   }
 }
 
 void SigninStatusMetricsProvider::UpdateStatusWhenBrowserAdded(bool signed_in) {
 #if !defined(OS_ANDROID)
   if ((signin_status_ == ALL_PROFILES_NOT_SIGNED_IN && signed_in) ||
       (signin_status_ == ALL_PROFILES_SIGNED_IN && !signed_in)) {
     signin_status_ = MIXED_SIGNIN_STATUS;
+  } else if (signin_status_ == UNKNOWN_SIGNIN_STATUS && signed_in) {

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

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]

Done.

+    signin_status_ = ALL_PROFILES_SIGNED_IN;
+  } else if (signin_status_ == UNKNOWN_SIGNIN_STATUS && !signed_in) {
+    signin_status_ = ALL_PROFILES_NOT_SIGNED_IN;
   }
 #endif
 }
 
 void SigninStatusMetricsProvider::ComputeCurrentSigninStatus() {
-  // Get the sign-in status of all currently open profiles. Sign-in status is
-  // indicated by its username. When username is not empty, the profile is
-  // signed-in.
   ProfileManager* profile_manager = g_browser_process->profile_manager();
   std::vector<Profile*> profile_list = profile_manager->GetLoadedProfiles();
 
   size_t opened_profiles_count = 0;
   size_t signed_in_profiles_count = 0;
 
   for (size_t i = 0; i < profile_list.size(); ++i) {
 #if !defined(OS_ANDROID)
     if (chrome::GetTotalBrowserCountForProfile(profile_list[i]) == 0) {
       // The profile is loaded, but there's no opened browser for this profile.
       continue;
     }
 #endif
     opened_profiles_count++;
     SigninManager* manager = SigninManagerFactory::GetForProfile(
         profile_list[i]->GetOriginalProfile());
     if (manager && manager->IsAuthenticated())
       signed_in_profiles_count++;
   }
-  UpdateInitialSigninStatus(opened_profiles_count, signed_in_profiles_count);
+
+  RecordComputeSigninStatusHistogram(ENTERED_COMPUTE_SIGNIN_STATUS);
+  if (profile_list.empty()) {
+    // This should not happen. If it does, record it in histogram.
+    RecordComputeSigninStatusHistogram(ERROR_NO_PROFILE_FOUND);
+    signin_status_ = ERROR_GETTING_SIGNIN_STATUS;
+  } else if (opened_profiles_count == 0) {
+    // The code indicates that Chrome is running in the background but no
+    // browser window is opened.
+    RecordComputeSigninStatusHistogram(NO_BROWSER_OPENED);
+    signin_status_ = UNKNOWN_SIGNIN_STATUS;
+  } else {
+    UpdateInitialSigninStatus(opened_profiles_count, signed_in_profiles_count);
+  }
 }
 
 SigninStatusMetricsProvider::ProfilesSigninStatus
 SigninStatusMetricsProvider::GetSigninStatusForTesting() {
   return signin_status_;
 }