Move content/child/background_sync to Blink.

third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp

+// Copyright 2015 The Chromium Authors. All rights reserved.

[haraken, 2016/11/05 13:00:18]

2016

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "modules/background_sync/BackgroundSyncProvider.h"
+
+#include "platform/WebTaskRunner.h"
+#include "public/platform/InterfaceProvider.h"
+#include "public/platform/Platform.h"
+#include "public/platform/modules/background_sync/WebSyncError.h"
+#include "wtf/Functional.h"
+#include <memory>
+#include <stddef.h>
+#include <utility>
+
+namespace blink {
+namespace {
+
+void ConnectToServiceOnMainThread(
+    mojom::blink::BackgroundSyncServiceRequest request) {
+  DCHECK(Platform::current());

[jbroman, 2016/11/05 22:51:09]

No need to check that Platform exists. If it doesn't, something went very bad a
long time ago already (and besides, we'll already get a crash on the next line
anyhow).

[adithyas, 2016/11/07 19:22:55]

Done.

+  Platform::current()->interfaceProvider()->getInterface(std::move(request));

[jbroman, 2016/11/05 22:51:09]

public/platform/InterfaceProvider.h promises to be thread-safe already (in fact,
it does a similar post to the main thread), so you should be able to just use
this directly in GetBackgroundSyncServicePtr

[adithyas, 2016/11/07 19:22:55]

Ah I see, did not know that, thanks!

+}
+
+}  // namespace
+
+void BackgroundSyncProvider::registerBackgroundSync(
+    mojom::blink::SyncRegistrationPtr& options,

[jbroman, 2016/11/05 22:51:09]

This is a reference-to-pointer; you should just use
mojom::blink::SyncRegistrationPtr. (You may need to add std::move to the call
site in order to do this).

[adithyas, 2016/11/07 19:22:55]

Done.

+    WebServiceWorkerRegistration* serviceWorkerRegistration,

[jbroman, 2016/11/05 22:51:08]

For legacy reasons, owning pointers were sometimes passed as raw pointers. This
parameter should probably be a unique_ptr (this will remove the need to create a
unique_ptr a few lines below). Similar elsewhere.

[adithyas, 2016/11/07 19:22:54]

Done.

+    SyncRegistrationCallbacks* callbacks) {
+  DCHECK(options);
+  DCHECK(serviceWorkerRegistration);
+  DCHECK(callbacks);
+  int64_t serviceWorkerRegistrationId =
+      serviceWorkerRegistration->registrationId();
+  std::unique_ptr<SyncRegistrationCallbacks> callbacksPtr(callbacks);
+
+  // WTF::unretained is safe here, as the mojo channel will be deleted (and
+  // will wipe its callbacks) before 'this' is deleted.

[haraken, 2016/11/05 13:00:18]

It may be safe but is not nice. See the below comment.

[adithyas, 2016/11/07 19:22:55]

Acknowledged.

+  GetBackgroundSyncServicePtr()->Register(
+      std::move(options), serviceWorkerRegistrationId,
+      convertToBaseCallback(WTF::bind(&BackgroundSyncProvider::RegisterCallback,
+                                      WTF::unretained(this),

[haraken, 2016/11/05 13:00:19]

We normally make the object GarbageCollected, but use wrapPersistent(this) to
keep the object alive until the callback is complete.

[jbroman, 2016/11/05 22:51:09]

Here and below, it doesn't look like any members are actually used, in which
case the methods could be made static (or file-local), and the |this| parameter
wouldn't be needed at all.

[adithyas, 2016/11/07 19:22:55]

Done.

+                                      WTF::passed(std::move(callbacksPtr)))));
+}
+
+void BackgroundSyncProvider::getRegistrations(
+    WebServiceWorkerRegistration* serviceWorkerRegistration,
+    SyncGetRegistrationsCallbacks* callbacks) {
+  DCHECK(serviceWorkerRegistration);
+  DCHECK(callbacks);
+  int64_t serviceWorkerRegistrationId =
+      serviceWorkerRegistration->registrationId();
+  std::unique_ptr<SyncGetRegistrationsCallbacks> callbacksPtr(callbacks);
+
+  // WTF::unretained is safe here, as the mojo channel will be deleted (and
+  // will wipe its callbacks) before 'this' is deleted.
+  GetBackgroundSyncServicePtr()->GetRegistrations(
+      serviceWorkerRegistrationId,
+      convertToBaseCallback(WTF::bind(
+          &BackgroundSyncProvider::GetRegistrationsCallback,
+          WTF::unretained(this), WTF::passed(std::move(callbacksPtr)))));

[haraken, 2016/11/05 13:00:19]

Ditto.

+}
+
+void BackgroundSyncProvider::RegisterCallback(

[jbroman, 2016/11/05 22:51:09]

(here and elsewhere) Blink method names should begin with a lowercase letter
(e.g. "registerCallback"). This may get rewritten in the future, but for now
camelCase is preferred for consistency.

[adithyas, 2016/11/07 19:22:55]

Done.

+    std::unique_ptr<SyncRegistrationCallbacks> callbacks,
+    mojom::blink::BackgroundSyncError error,
+    mojom::blink::SyncRegistrationPtr options) {
+  // TODO(iclelland): Determine the correct error message to return in each case
+  switch (error) {
+    case mojom::blink::BackgroundSyncError::NONE:
+      if (!options.is_null())
+        callbacks->onSuccess(std::move(options));
+      break;
+    case mojom::blink::BackgroundSyncError::NOT_FOUND:
+      NOTREACHED();
+      break;
+    case mojom::blink::BackgroundSyncError::STORAGE:
+      callbacks->onError(blink::WebSyncError(WebSyncError::ErrorTypeUnknown,
+                                             "Background Sync is disabled."));
+      break;
+    case mojom::blink::BackgroundSyncError::NOT_ALLOWED:
+      callbacks->onError(
+          blink::WebSyncError(WebSyncError::ErrorTypeNoPermission,
+                              "Attempted to register a sync event without a "
+                              "window or registration tag too long."));
+      break;
+    case mojom::blink::BackgroundSyncError::PERMISSION_DENIED:
+      callbacks->onError(blink::WebSyncError(
+          WebSyncError::ErrorTypePermissionDenied, "Permission denied."));
+      break;
+    case mojom::blink::BackgroundSyncError::NO_SERVICE_WORKER:
+      callbacks->onError(blink::WebSyncError(WebSyncError::ErrorTypeUnknown,
+                                             "No service worker is active."));
+      break;

[haraken, 2016/11/05 13:00:19]

Shall we add:

  default:
    NOTREACHED();

?

[jbroman, 2016/11/05 22:51:09]

Doesn't doing so turn a compile-time error (not exhaustively covering the enum)
into a runtime error? (here and below)

+  }
+}
+
+void BackgroundSyncProvider::GetRegistrationsCallback(
+    std::unique_ptr<SyncGetRegistrationsCallbacks> syncGetRegistrationCallbacks,
+    mojom::blink::BackgroundSyncError error,
+    mojo::WTFArray<mojom::blink::SyncRegistrationPtr> registrations) {
+  // TODO(iclelland): Determine the correct error message to return in each case
+  switch (error) {
+    case mojom::blink::BackgroundSyncError::NONE: {
+      WebVector<mojom::blink::SyncRegistration*> results(registrations.size());

[jbroman, 2016/11/05 22:51:08]

In a followup CL, you can switch from WebVector to
Vector<mojom::blink::SyncRegistrationPtr>, which is what WTFArray is backed by
anyhow (registrations.storage() will give you a const&, or you can pass
ownership out of the WTFArray if needed).

[adithyas, 2016/11/07 19:22:55]

Acknowledged.

+      for (size_t i = 0; i < registrations.size(); ++i) {
+        results[i] = registrations[i].get();
+      }
+      syncGetRegistrationCallbacks->onSuccess(results);
+      break;
+    }
+    case mojom::blink::BackgroundSyncError::NOT_FOUND:
+    case mojom::blink::BackgroundSyncError::NOT_ALLOWED:
+    case mojom::blink::BackgroundSyncError::PERMISSION_DENIED:
+      // These errors should never be returned from
+      // BackgroundSyncManager::GetRegistrations
+      NOTREACHED();
+      break;
+    case mojom::blink::BackgroundSyncError::STORAGE:
+      syncGetRegistrationCallbacks->onError(blink::WebSyncError(

[jbroman, 2016/11/05 22:51:08]

Similarly, it looks like WebSyncError can move to modules/background_sync/ in a
followup CL (and switch from WebString to String).

+          WebSyncError::ErrorTypeUnknown, "Background Sync is disabled."));
+      break;
+    case mojom::blink::BackgroundSyncError::NO_SERVICE_WORKER:
+      syncGetRegistrationCallbacks->onError(blink::WebSyncError(
+          WebSyncError::ErrorTypeUnknown, "No service worker is active."));
+      break;

[haraken, 2016/11/05 13:00:19]

Ditto.

+  }
+}
+
+mojom::blink::BackgroundSyncServicePtr&
+BackgroundSyncProvider::GetBackgroundSyncServicePtr() {
+  if (!backgroundSyncService.get()) {
+    mojo::InterfaceRequest<mojom::blink::BackgroundSyncService> request =
+        mojo::GetProxy(&backgroundSyncService);
+    Platform::current()->mainThread()->getWebTaskRunner()->postTask(

[haraken, 2016/11/05 13:00:18]

Why do we need to post a task?

[jbroman, 2016/11/05 22:51:08]

I commented on this above. The content-side equivalent of this class isn't
thread-safe, but the Blink-side one is (and in fact, is implemented with a
post-to-main).

+        BLINK_FROM_HERE, WTF::bind(&ConnectToServiceOnMainThread,
+                                   WTF::passed(std::move(request))));
+  }
+  return backgroundSyncService;
+}
+
+}  // namespace blink