[Background Sync] Add proper error status to API calls and callbacks

content/child/background_sync/background_sync_provider.cc

 // Copyright 2015 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 "content/child/background_sync/background_sync_provider.h"
 
 #include "base/bind.h"
 #include "base/memory/scoped_ptr.h"
 #include "content/child/background_sync/background_sync_type_converters.h"
 #include "content/child/service_worker/web_service_worker_registration_impl.h"
@@ skipping 100 matching lines @@
   // will wipe its callbacks) before 'this' is deleted.
   GetBackgroundSyncServicePtr()->GetRegistrations(
       mojo::ConvertTo<BackgroundSyncPeriodicity>(periodicity),
       service_worker_registration_id,
       base::Bind(&BackgroundSyncProvider::GetRegistrationsCallback,
                  base::Unretained(this), base::Passed(callbacksPtr.Pass())));
 }
 
 void BackgroundSyncProvider::RegisterCallback(
     scoped_ptr<blink::WebSyncRegistrationCallbacks> callbacks,
+    BackgroundSyncError error,
     const SyncRegistrationPtr& options) {
-  // TODO(iclelland): Pass through the various errors from the manager to here
-  // and handle them appropriately.
+  // TODO(iclelland): Determine the correct error message to return in each case
   scoped_ptr<blink::WebSyncRegistration> result;
-  if (!options.is_null())
-    result = mojo::ConvertTo<scoped_ptr<blink::WebSyncRegistration>>(options);
-
-  callbacks->onSuccess(result.release());
+  switch (error) {
+    case BACKGROUND_SYNC_ERROR_NONE:
+      if (!options.is_null())
+        result =

[jkarlin, 2015/05/13 11:19:46]

Might as well declare result here as it's not used anywhere else.

[iclelland, 2015/05/13 11:48:36]

clang doesn't allow variable declaration from within a switch case :( It's not a
scope.

I could either introduce an anonymous scope just for the case, or, if null is
never a valid possibility for an ERROR_NONE result, I could check for that
explicitly, and declare result in the scope of the if clause.

[jkarlin, 2015/05/13 12:01:56]

My bad, what you have is fine.

+            mojo::ConvertTo<scoped_ptr<blink::WebSyncRegistration>>(options);
+      callbacks->onSuccess(result.release());
+      break;
+    case BACKGROUND_SYNC_ERROR_NOT_FOUND:
+      callbacks->onSuccess(nullptr);

[jkarlin, 2015/05/13 11:19:46]

This should be a NOTREACHED() case

[iclelland, 2015/05/13 11:48:36]

Perhaps I should rename the method instead; the same callback is used for both
register() and getRegistrations(), where NOT_FOUND is expected.

[jkarlin, 2015/05/13 12:01:56]

I see. Yes, I'd split them into different callbacks as they expect different
errors.

[iclelland, 2015/05/13 12:18:32]

Done.

+      break;
+    case BACKGROUND_SYNC_ERROR_STORAGE:
+      callbacks->onError(
+          new blink::WebSyncError(blink::WebSyncError::ErrorTypeUnknown,
+                                  "Background Sync is disabled."));
+      break;
+    case BACKGROUND_SYNC_ERROR_NO_SERVICE_WORKER:
+      callbacks->onError(
+          new blink::WebSyncError(blink::WebSyncError::ErrorTypeUnknown,
+                                  "No service worker is active."));
+      break;
+  }
 }
 
 void BackgroundSyncProvider::UnregisterCallback(
     scoped_ptr<blink::WebSyncUnregistrationCallbacks> callbacks,
-    bool success) {
-  // TODO(iclelland): Pass through the various errors from the manager to here
-  // and handle them appropriately.
-  if (success) {
-    callbacks->onSuccess(new bool(success));
-  } else {
-    callbacks->onError(
-        new blink::WebSyncError(blink::WebSyncError::ErrorTypeUnknown,
-                                "Sync registration does not exist"));
+    BackgroundSyncError error) {
+  // TODO(iclelland): Determine the correct error message to return in each case
+  switch (error) {
+    case BACKGROUND_SYNC_ERROR_NONE:
+      callbacks->onSuccess(new bool(true));
+      break;
+    case BACKGROUND_SYNC_ERROR_NOT_FOUND:
+      callbacks->onSuccess(new bool(false));
+      break;
+    case BACKGROUND_SYNC_ERROR_STORAGE:
+      callbacks->onError(
+          new blink::WebSyncError(blink::WebSyncError::ErrorTypeUnknown,
+                                  "Background Sync is disabled."));
+      break;
+    case BACKGROUND_SYNC_ERROR_NO_SERVICE_WORKER:
+      callbacks->onError(
+          new blink::WebSyncError(blink::WebSyncError::ErrorTypeUnknown,
+                                  "No service worker is active."));
+      break;
   }
 }
 
 void BackgroundSyncProvider::GetRegistrationsCallback(
     scoped_ptr<blink::WebSyncGetRegistrationsCallbacks> callbacks,
+    BackgroundSyncError error,
     const mojo::Array<SyncRegistrationPtr>& registrations) {
-  // TODO(iclelland): Pass through the various errors from the manager to here
-  // and handle them appropriately.
-  blink::WebVector<blink::WebSyncRegistration*>* results =
-      new blink::WebVector<blink::WebSyncRegistration*>(registrations.size());
-  for (size_t i = 0; i < registrations.size(); ++i) {
-    (*results)[i] = mojo::ConvertTo<scoped_ptr<blink::WebSyncRegistration>>(
-                        registrations[i]).release();
+  // TODO(iclelland): Determine the correct error message to return in each case
+  blink::WebVector<blink::WebSyncRegistration*>* results;
+  switch (error) {
+    case BACKGROUND_SYNC_ERROR_NONE:
+      results = new blink::WebVector<blink::WebSyncRegistration*>(
+          registrations.size());

[jkarlin, 2015/05/13 11:19:46]

Results should be declared here

[jkarlin, 2015/05/13 12:01:56]

I retract this comment, you can't declare it within the switch.

+      for (size_t i = 0; i < registrations.size(); ++i) {
+        (*results)[i] = mojo::ConvertTo<scoped_ptr<blink::WebSyncRegistration>>(
+                            registrations[i]).release();
+      }
+      callbacks->onSuccess(results);
+    case BACKGROUND_SYNC_ERROR_NOT_FOUND:
+      // This error should never be returned from
+      // BackgroundSyncManager::GetRegistrations
+      NOTREACHED();
+      break;
+    case BACKGROUND_SYNC_ERROR_STORAGE:
+      callbacks->onError(
+          new blink::WebSyncError(blink::WebSyncError::ErrorTypeUnknown,
+                                  "Background Sync is disabled."));
+      break;
+    case BACKGROUND_SYNC_ERROR_NO_SERVICE_WORKER:
+      callbacks->onError(
+          new blink::WebSyncError(blink::WebSyncError::ErrorTypeUnknown,
+                                  "No service worker is active."));
+      break;
   }
-  callbacks->onSuccess(results);
 }
 
 BackgroundSyncServicePtr&
 BackgroundSyncProvider::GetBackgroundSyncServicePtr() {
   if (!background_sync_service_.get())
     service_registry_->ConnectToRemoteService(&background_sync_service_);
   return background_sync_service_;
 }
 
 }  // namespace content