[BackgroundSync] Split failed and succeeded cases of BackgroundSyncMetrics::CountRegister

Doing this prevents us from logging bogus values (for duplicate and could_fire) in case of failure.

BUG=559143

Review URL: https://codereview.chromium.org/1465533006

Cr-Commit-Position: refs/heads/master@{#361194}

3 files changed, 64 insertions, 60 deletions

diff --git a/content/browser/background_sync/background_sync_manager.cc b/content/browser/background_sync/background_sync_manager.cc
index 97b637f..071b97a 100644
--- a/content/browser/background_sync/background_sync_manager.cc
+++ b/content/browser/background_sync/background_sync_manager.cc
@@ -174,17 +174,9 @@ void BackgroundSyncManager::Register(
     const StatusAndRegistrationCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  // For UMA, determine here whether the sync could fire immediately
-  BackgroundSyncMetrics::RegistrationCouldFire registration_could_fire =
-      AreOptionConditionsMet(options)
-          ? BackgroundSyncMetrics::REGISTRATION_COULD_FIRE
-          : BackgroundSyncMetrics::REGISTRATION_COULD_NOT_FIRE;
-
   if (disabled_) {
-    BackgroundSyncMetrics::CountRegister(
-        options.periodicity, registration_could_fire,
-        BackgroundSyncMetrics::REGISTRATION_IS_NOT_DUPLICATE,
-        BACKGROUND_SYNC_STATUS_STORAGE_ERROR);
+    BackgroundSyncMetrics::CountRegisterFailure(
+        options.periodicity, BACKGROUND_SYNC_STATUS_STORAGE_ERROR);
     PostErrorResponse(BACKGROUND_SYNC_STATUS_STORAGE_ERROR, callback);
     return;
   }
@@ -407,17 +399,9 @@ void BackgroundSyncManager::RegisterImpl(
     const StatusAndRegistrationCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  // For UMA, determine here whether the sync could fire immediately
-  BackgroundSyncMetrics::RegistrationCouldFire registration_could_fire =
-      AreOptionConditionsMet(options)
-          ? BackgroundSyncMetrics::REGISTRATION_COULD_FIRE
-          : BackgroundSyncMetrics::REGISTRATION_COULD_NOT_FIRE;
-
   if (disabled_) {
-    BackgroundSyncMetrics::CountRegister(
-        options.periodicity, registration_could_fire,
-        BackgroundSyncMetrics::REGISTRATION_IS_NOT_DUPLICATE,
-        BACKGROUND_SYNC_STATUS_STORAGE_ERROR);
+    BackgroundSyncMetrics::CountRegisterFailure(
+        options.periodicity, BACKGROUND_SYNC_STATUS_STORAGE_ERROR);
     PostErrorResponse(BACKGROUND_SYNC_STATUS_STORAGE_ERROR, callback);
     return;
   }
@@ -430,10 +414,8 @@ void BackgroundSyncManager::RegisterImpl(
   }
 
   if (options.tag.length() > kMaxTagLength) {
-    BackgroundSyncMetrics::CountRegister(
-        options.periodicity, registration_could_fire,
-        BackgroundSyncMetrics::REGISTRATION_IS_NOT_DUPLICATE,
-        BACKGROUND_SYNC_STATUS_NOT_ALLOWED);
+    BackgroundSyncMetrics::CountRegisterFailure(
+        options.periodicity, BACKGROUND_SYNC_STATUS_NOT_ALLOWED);
     PostErrorResponse(BACKGROUND_SYNC_STATUS_NOT_ALLOWED, callback);
     return;
   }
@@ -441,10 +423,8 @@ void BackgroundSyncManager::RegisterImpl(
   ServiceWorkerRegistration* sw_registration =
       service_worker_context_->GetLiveRegistration(sw_registration_id);
   if (!sw_registration || !sw_registration->active_version()) {
-    BackgroundSyncMetrics::CountRegister(
-        options.periodicity, registration_could_fire,
-        BackgroundSyncMetrics::REGISTRATION_IS_NOT_DUPLICATE,
-        BACKGROUND_SYNC_STATUS_NO_SERVICE_WORKER);
+    BackgroundSyncMetrics::CountRegisterFailure(
+        options.periodicity, BACKGROUND_SYNC_STATUS_NO_SERVICE_WORKER);
     PostErrorResponse(BACKGROUND_SYNC_STATUS_NO_SERVICE_WORKER, callback);
     return;
   }
@@ -468,11 +448,14 @@ void BackgroundSyncManager::RegisterImpl(
       BackgroundSyncRegistration* existing_registration =
           existing_registration_ref->value();
 
-    // Record the duplicated registration
-    BackgroundSyncMetrics::CountRegister(
-        existing_registration->options()->periodicity, registration_could_fire,
-        BackgroundSyncMetrics::REGISTRATION_IS_DUPLICATE,
-        BACKGROUND_SYNC_STATUS_OK);
+      BackgroundSyncMetrics::RegistrationCouldFire registration_could_fire =
+          AreOptionConditionsMet(options)
+              ? BackgroundSyncMetrics::REGISTRATION_COULD_FIRE
+              : BackgroundSyncMetrics::REGISTRATION_COULD_NOT_FIRE;
+      BackgroundSyncMetrics::CountRegisterSuccess(
+          existing_registration->options()->periodicity,
+          registration_could_fire,
+          BackgroundSyncMetrics::REGISTRATION_IS_DUPLICATE);
 
     if (existing_registration->IsFiring()) {
       existing_registration->set_sync_state(
@@ -646,17 +629,10 @@ void BackgroundSyncManager::RegisterDidStore(
   const BackgroundSyncRegistration* new_registration =
       new_registration_ref->value();
 
-  // For UMA, determine here whether the sync could fire immediately
-  BackgroundSyncMetrics::RegistrationCouldFire registration_could_fire =
-      AreOptionConditionsMet(*new_registration->options())
-          ? BackgroundSyncMetrics::REGISTRATION_COULD_FIRE
-          : BackgroundSyncMetrics::REGISTRATION_COULD_NOT_FIRE;
-
   if (status == SERVICE_WORKER_ERROR_NOT_FOUND) {
     // The service worker registration is gone.
-    BackgroundSyncMetrics::CountRegister(
-        new_registration->options()->periodicity, registration_could_fire,
-        BackgroundSyncMetrics::REGISTRATION_IS_NOT_DUPLICATE,
+    BackgroundSyncMetrics::CountRegisterFailure(
+        new_registration->options()->periodicity,
         BACKGROUND_SYNC_STATUS_STORAGE_ERROR);
     active_registrations_.erase(sw_registration_id);
     PostErrorResponse(BACKGROUND_SYNC_STATUS_STORAGE_ERROR, callback);
@@ -666,9 +642,8 @@ void BackgroundSyncManager::RegisterDidStore(
   if (status != SERVICE_WORKER_OK) {
     LOG(ERROR) << "BackgroundSync failed to store registration due to backend "
                   "failure.";
-    BackgroundSyncMetrics::CountRegister(
-        new_registration->options()->periodicity, registration_could_fire,
-        BackgroundSyncMetrics::REGISTRATION_IS_NOT_DUPLICATE,
+    BackgroundSyncMetrics::CountRegisterFailure(
+        new_registration->options()->periodicity,
         BACKGROUND_SYNC_STATUS_STORAGE_ERROR);
     DisableAndClearManager(base::Bind(
         callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR,
@@ -676,10 +651,14 @@ void BackgroundSyncManager::RegisterDidStore(
     return;
   }
 
-  BackgroundSyncMetrics::CountRegister(
+  BackgroundSyncMetrics::RegistrationCouldFire registration_could_fire =
+      AreOptionConditionsMet(*new_registration->options())
+          ? BackgroundSyncMetrics::REGISTRATION_COULD_FIRE
+          : BackgroundSyncMetrics::REGISTRATION_COULD_NOT_FIRE;
+  BackgroundSyncMetrics::CountRegisterSuccess(
       new_registration->options()->periodicity, registration_could_fire,
-      BackgroundSyncMetrics::REGISTRATION_IS_NOT_DUPLICATE,
-      BACKGROUND_SYNC_STATUS_OK);
+      BackgroundSyncMetrics::REGISTRATION_IS_NOT_DUPLICATE);
+
   FireReadyEvents();
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
diff --git a/content/browser/background_sync/background_sync_metrics.cc b/content/browser/background_sync/background_sync_metrics.cc
index 4bfef5c..dddec9b 100644
--- a/content/browser/background_sync/background_sync_metrics.cc
+++ b/content/browser/background_sync/background_sync_metrics.cc
@@ -32,6 +32,7 @@ ResultPattern EventResultToResultPattern(bool success,
 
 namespace content {
 
+// static
 void BackgroundSyncMetrics::RecordEventResult(SyncPeriodicity periodicity,
                                               bool success,
                                               bool finished_in_foreground) {
@@ -52,6 +53,7 @@ void BackgroundSyncMetrics::RecordEventResult(SyncPeriodicity periodicity,
   NOTREACHED();
 }
 
+// static
 void BackgroundSyncMetrics::RecordBatchSyncEventComplete(
     const base::TimeDelta& time,
     int number_of_batched_sync_events) {
@@ -64,14 +66,15 @@ void BackgroundSyncMetrics::RecordBatchSyncEventComplete(
                            number_of_batched_sync_events);
 }
 
-void BackgroundSyncMetrics::CountRegister(
+// static
+void BackgroundSyncMetrics::CountRegisterSuccess(
     SyncPeriodicity periodicity,
     RegistrationCouldFire registration_could_fire,
-    RegistrationIsDuplicate registration_is_duplicate,
-    BackgroundSyncStatus result) {
+    RegistrationIsDuplicate registration_is_duplicate) {
   switch (periodicity) {
     case SYNC_ONE_SHOT:
-      UMA_HISTOGRAM_ENUMERATION("BackgroundSync.Registration.OneShot", result,
+      UMA_HISTOGRAM_ENUMERATION("BackgroundSync.Registration.OneShot",
+                                BACKGROUND_SYNC_STATUS_OK,
                                 BACKGROUND_SYNC_STATUS_MAX + 1);
       UMA_HISTOGRAM_BOOLEAN("BackgroundSync.Registration.OneShot.CouldFire",
                             registration_could_fire == REGISTRATION_COULD_FIRE);
@@ -80,7 +83,8 @@ void BackgroundSyncMetrics::CountRegister(
           registration_is_duplicate == REGISTRATION_IS_DUPLICATE);
       return;
     case SYNC_PERIODIC:
-      UMA_HISTOGRAM_ENUMERATION("BackgroundSync.Registration.Periodic", result,
+      UMA_HISTOGRAM_ENUMERATION("BackgroundSync.Registration.Periodic",
+                                BACKGROUND_SYNC_STATUS_OK,
                                 BACKGROUND_SYNC_STATUS_MAX + 1);
       UMA_HISTOGRAM_BOOLEAN(
           "BackgroundSync.Registration.Periodic.IsDuplicate",
@@ -90,6 +94,23 @@ void BackgroundSyncMetrics::CountRegister(
   NOTREACHED();
 }
 
+// static
+void BackgroundSyncMetrics::CountRegisterFailure(SyncPeriodicity periodicity,
+                                                 BackgroundSyncStatus result) {
+  switch (periodicity) {
+    case SYNC_ONE_SHOT:
+      UMA_HISTOGRAM_ENUMERATION("BackgroundSync.Registration.OneShot", result,
+                                BACKGROUND_SYNC_STATUS_MAX + 1);
+      return;
+    case SYNC_PERIODIC:
+      UMA_HISTOGRAM_ENUMERATION("BackgroundSync.Registration.Periodic", result,
+                                BACKGROUND_SYNC_STATUS_MAX + 1);
+      return;
+  }
+  NOTREACHED();
+}
+
+// static
 void BackgroundSyncMetrics::CountUnregister(SyncPeriodicity periodicity,
                                             BackgroundSyncStatus result) {
   switch (periodicity) {
diff --git a/content/browser/background_sync/background_sync_metrics.h b/content/browser/background_sync/background_sync_metrics.h
index 440ce3f..2c958b3 100644
--- a/content/browser/background_sync/background_sync_metrics.h
+++ b/content/browser/background_sync/background_sync_metrics.h
@@ -38,13 +38,17 @@ class BackgroundSyncMetrics {
   static void RecordBatchSyncEventComplete(const base::TimeDelta& time,
                                            int number_of_batched_sync_events);
 
-  // Records the result of trying to register a sync. |could_fire| indicates
-  // whether the conditions were sufficient for the sync to fire immediately at
-  // the time it was registered.
-  static void CountRegister(SyncPeriodicity periodicity,
-                            RegistrationCouldFire could_fire,
-                            RegistrationIsDuplicate registration_is_duplicate,
-                            BackgroundSyncStatus result);
+  // Records the result of successfully registering a sync. |could_fire|
+  // indicates whether the conditions were sufficient for the sync to fire
+  // immediately at the time it was registered.
+  static void CountRegisterSuccess(
+      SyncPeriodicity periodicity,
+      RegistrationCouldFire could_fire,
+      RegistrationIsDuplicate registration_is_duplicate);
+
+  // Records the status of a failed sync registration.
+  static void CountRegisterFailure(SyncPeriodicity periodicity,
+                                   BackgroundSyncStatus status);
 
   // Records the result of trying to unregister a sync.
   static void CountUnregister(SyncPeriodicity periodicity,