[background-sync] Remove WebSyncError and SyncCallbacks

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

Index: third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h
diff --git a/third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h b/third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h
index af633217d1cf1a053375f3677dd977afc007880c..e3fa70b282d1cfe70cab07313d2ee303223fe223 100644
--- a/third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h
+++ b/third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h
@@ -5,7 +5,6 @@
 #ifndef BackgroundSyncProvider_h
 #define BackgroundSyncProvider_h
 
-#include "modules/background_sync/SyncCallbacks.h"
 #include "public/platform/modules/background_sync/background_sync.mojom-blink.h"
 #include "wtf/Noncopyable.h"
 #include <memory>

[jbroman, 2016/11/18 16:53:32]

super-nit: I think this include was here for unique_ptr, which is no longer used

[adithyas, 2016/11/18 20:05:00]

Done.

@@ -13,6 +12,8 @@
 namespace blink {
 
 class WebServiceWorkerRegistration;
+class ScriptPromiseResolver;

[jbroman, 2016/11/18 16:53:31]

nit: alphabetize these

[adithyas, 2016/11/18 20:05:00]

Done.

+class SyncError;
 
 // The BackgroundSyncProvider is called by the SyncManager and SyncRegistration
 // objects and communicates with the BackgroundSyncManager object in the browser
@@ -28,21 +29,28 @@ class BackgroundSyncProvider {
 
   void registerBackgroundSync(mojom::blink::SyncRegistrationPtr options,
                               WebServiceWorkerRegistration*,
-                              std::unique_ptr<SyncRegistrationCallbacks>);
-  void getRegistrations(WebServiceWorkerRegistration*,
-                        std::unique_ptr<SyncGetRegistrationsCallbacks>);
+                              ScriptPromiseResolver*);

[jbroman, 2016/11/18 16:53:32]

Since there's only the one implementation of Sync*Callbacks, this seems like a
reasonable simplification to make. It does bind this class fairly closely to the
interface of SyncManager. WDYT of (probably in a followup, since it changes
lifetime semantics) merging this class into SyncManager?

That way, the method that creates the promise and the one that resolves it live
in the same class, so it's easier to see at a glance what values the promise can
be resolved/rejected with.

[adithyas, 2016/11/18 20:05:00]

Yup I definitely agree with that thought, both classes basically have identical
interfaces. This was something I wanted to change in this CL, but like you
mentioned, it changes lifetime semantics with BackgroundSyncProvider currently
having thread specific instances. 

I also wanted to know the owners' thoughts about this before changing the
implementation.

[iclelland, 2016/11/18 20:53:46]

The difference, I believe, is that there can be multiple SyncManagers present in
the main thread -- up to one per ServiceWorkerRegistration for the page's
origin. They all use the same BackgroundSyncProvider, though, since we only need
one IPC channel between the renderer and the browser.

[jbroman, 2016/11/19 03:32:15]

Right, that's the difference (unless there's a correctness difference I'm
missing).

Personally I'd prefer to simplify here. IIUC Mojo pipes are fairly inexpensive,
and it makes the lifetime easier to reason about without having an thread-local
object to track, as well as putting the code from JS bindings to Mojo call and
back again in one place. But you're the modules/background_sync/ owner, so it
probably matters more what you think. :)

[haraken, 2016/11/21 00:21:58]

+1

+  void getRegistrations(WebServiceWorkerRegistration*, ScriptPromiseResolver*);
 
  private:
   // Callback handlers
-  static void registerCallback(
-      std::unique_ptr<blink::SyncRegistrationCallbacks>,
-      mojom::blink::BackgroundSyncError,
-      mojom::blink::SyncRegistrationPtr options);
+  static void registerCallback(ScriptPromiseResolver*,
+                               mojom::blink::BackgroundSyncError,
+                               mojom::blink::SyncRegistrationPtr options);
   static void getRegistrationsCallback(
-      std::unique_ptr<SyncGetRegistrationsCallbacks>,
+      ScriptPromiseResolver*,
       mojom::blink::BackgroundSyncError,
       mojo::WTFArray<mojom::blink::SyncRegistrationPtr> registrations);
 
+  // Success and error handlers
+  static void onRegisterSuccess(ScriptPromiseResolver*,
+                                mojom::blink::SyncRegistrationPtr);
+  static void onRegisterError(ScriptPromiseResolver*, const SyncError&);
+  static void onGetRegistrationsSuccess(
+      ScriptPromiseResolver*,
+      const Vector<mojom::blink::SyncRegistrationPtr>&);
+  static void onGetRegistrationsError(ScriptPromiseResolver*, const SyncError&);
+
   // Returns an initialized BackgroundSyncServicePtr. A connection with the
   // the browser's BackgroundSyncService is created the first time this method
   // is called.