Implement BackgroundFetchManager.fetch() and struct traits

third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp

Index: third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp
diff --git a/third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp b/third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp
index ef89640d0c848985c086ad7795aa43295e7fecae..c8b0382588be2782cf8f08ffc8ab916d7e8fee66 100644
--- a/third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp
+++ b/third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp
@@ -7,6 +7,7 @@
 #include <utility>
 #include "modules/background_fetch/BackgroundFetchOptions.h"
 #include "modules/background_fetch/BackgroundFetchRegistration.h"
+#include "modules/background_fetch/BackgroundFetchTypeConverters.h"

[dcheng, 2017/03/21 07:03:20]

Let's use typemapping on the Blink side. It'll save some temporaries and be a
bit more efficient.

For a relatively simple example,
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/mojo/...
should provide some useful pointers. I'm happy to provide detailed help as well
if there's issues getting this to work.

[Peter Beverloo, 2017/03/21 14:05:19]

These types live in //Source/modules/, whereas the mojom lives in
//public/platform/ and are, on the Blink side, linked in as part of
//Source/platform/. The issue is that platform cannot depend on modules.

One approach would be to define another layer of wrapper types in platform that
we provide a typemap for, which the module then uses to create the actual types.
That seems unfortunate as it's more duplication.

Another approach would be to separate out the background fetch mojom (and
potentially other affected mojom files, e.g. NFC) to a new target that still
lives in //public/platform/, but whose Blink bindings will actually be linked in
with modules. That feels a bit icky as it requires the Blink embedder to know
about this distinction by depending on another target.

I'm not sure what the best way forward here is. What's your take?

 #include "modules/background_fetch/IconDefinition.h"
 #include "public/platform/InterfaceProvider.h"
 #include "public/platform/Platform.h"
@@ -14,31 +15,6 @@
 
 namespace blink {
 
-namespace {
-
-// Creates a new BackgroundFetchRegistration instance given a Service Worker
-// Registration and a Mojo BackgroundFetchRegistrationPtr instance.
-BackgroundFetchRegistration* CreateBackgroundFetchRegistration(
-    ServiceWorkerRegistration* serviceWorkerRegistration,
-    mojom::blink::BackgroundFetchRegistrationPtr registrationPtr) {
-  HeapVector<IconDefinition> icons;
-
-  for (const auto& iconPtr : registrationPtr->icons) {
-    IconDefinition icon;
-    icon.setSrc(iconPtr->src);
-    icon.setSizes(iconPtr->sizes);
-    icon.setType(iconPtr->type);
-
-    icons.push_back(icon);
-  }
-
-  return new BackgroundFetchRegistration(
-      serviceWorkerRegistration, registrationPtr->tag, std::move(icons),
-      registrationPtr->total_download_size, registrationPtr->title);
-}
-
-}  // namespace
-
 // static
 BackgroundFetchBridge* BackgroundFetchBridge::from(
     ServiceWorkerRegistration* serviceWorkerRegistration) {
@@ -68,6 +44,18 @@ BackgroundFetchBridge::BackgroundFetchBridge(
 
 BackgroundFetchBridge::~BackgroundFetchBridge() = default;
 
+void BackgroundFetchBridge::fetch(
+    const String& tag,
+    const BackgroundFetchOptions& options,
+    std::unique_ptr<RegistrationCallback> callback) {
+  getService()->Fetch(
+      supplementable()->webRegistration()->registrationId(), tag,
+      mojom::blink::BackgroundFetchOptions::From(options),
+      convertToBaseCallback(
+          WTF::bind(&BackgroundFetchBridge::didGetRegistration,
+                    wrapPersistent(this), WTF::passed(std::move(callback)))));
+}
+
 void BackgroundFetchBridge::abort(const String& tag) {
   getService()->Abort(supplementable()->webRegistration()->registrationId(),
                       tag);
@@ -84,7 +72,7 @@ void BackgroundFetchBridge::updateUI(
 
 void BackgroundFetchBridge::getRegistration(
     const String& tag,
-    std::unique_ptr<GetRegistrationCallback> callback) {
+    std::unique_ptr<RegistrationCallback> callback) {
   getService()->GetRegistration(
       supplementable()->webRegistration()->registrationId(), tag,
       convertToBaseCallback(
@@ -93,15 +81,15 @@ void BackgroundFetchBridge::getRegistration(
 }
 
 void BackgroundFetchBridge::didGetRegistration(

[harkness, 2017/03/21 11:30:00]

optional: I'm glad that we're reusing the code for both fetch and
getRegistration, but consider renaming didGetRegistration to something more
generic so it doesn't look like a 1:1 mapping to getRegistration.

[Peter Beverloo, 2017/03/21 13:46:27]

Can you suggest a name that you think would be clearer? While I agree that the
overlap with the getRegistration() method is unfortunate, in both cases we did
get a registration.

-    std::unique_ptr<GetRegistrationCallback> callback,
+    std::unique_ptr<RegistrationCallback> callback,
     mojom::blink::BackgroundFetchError error,
     mojom::blink::BackgroundFetchRegistrationPtr registrationPtr) {
-  BackgroundFetchRegistration* registration = nullptr;
+  BackgroundFetchRegistration* registration =
+      registrationPtr.To<BackgroundFetchRegistration*>();
 
-  if (registrationPtr) {
+  if (registration) {
     DCHECK_EQ(error, mojom::blink::BackgroundFetchError::NONE);
-    registration = CreateBackgroundFetchRegistration(
-        supplementable(), std::move(registrationPtr));
+    registration->setServiceWorkerRegistration(supplementable());
   }
 
   (*callback)(error, registration);