Push API: move PushManager from Navigator to ServiceWorkerRegistration [switchover 6/6]

Source/modules/push_messaging/PushManager.cpp

Index: Source/modules/push_messaging/PushManager.cpp
diff --git a/Source/modules/push_messaging/PushManager.cpp b/Source/modules/push_messaging/PushManager.cpp
index d84587e8e98484cc502c3c242aafdf41957e7328..42a6c32ae9853319759f550a6ad536015d8c033e 100644
--- a/Source/modules/push_messaging/PushManager.cpp
+++ b/Source/modules/push_messaging/PushManager.cpp
@@ -20,63 +20,57 @@
 #include "modules/push_messaging/PushRegistration.h"
 #include "modules/serviceworkers/NavigatorServiceWorker.h"

[Peter Beverloo, 2014/12/08 21:40:51]

nit: This include seems to be unused.

[Michael van Ouwerkerk, 2014/12/09 18:47:45]

Could have sworn I removed that. Maybe I mishandled some merges. Done.

 #include "modules/serviceworkers/ServiceWorkerContainer.h"

[Peter Beverloo, 2014/12/08 21:40:51]

nit: This include seems to be unused.

[Michael van Ouwerkerk, 2014/12/09 18:47:45]

Done.

+#include "modules/serviceworkers/ServiceWorkerRegistration.h"
+#include "public/platform/Platform.h"
 #include "public/platform/WebPushClient.h"
+#include "public/platform/WebPushProvider.h"
 #include "wtf/RefPtr.h"
 
 namespace blink {
 
-PushManager::PushManager()
+PushManager::PushManager(ServiceWorkerRegistration* registration)
+    : m_registration(registration)
 {

[Peter Beverloo, 2014/12/08 21:40:51]

ASSERT(registration)

[Michael van Ouwerkerk, 2014/12/09 18:47:45]

Done.

 }
 
-// FIXME: This call should be available from workers which will not have a Document object available.
-// See crbug.com/389194
 ScriptPromise PushManager::registerPushMessaging(ScriptState* scriptState)
 {
-    ASSERT(scriptState->executionContext()->isDocument());
-
-    Document* document = toDocument(scriptState->executionContext());
-    if (!document->domWindow() || !document->frame())
-        return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(AbortError, "Document is detached from window."));
-
-    WebServiceWorkerProvider* serviceWorkerProvider = NavigatorServiceWorker::serviceWorker(*document->domWindow()->navigator())->provider();
-    if (!serviceWorkerProvider)
-        return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(AbortError, "No Service Worker installed for this document."));
-
-    WebPushClient* client = PushController::clientFrom(document->frame());
-    ASSERT(client);
+    if (scriptState->executionContext()->isDocument()) {
+        Document* document = toDocument(scriptState->executionContext());
+        if (!document->domWindow() || !document->frame())
+            return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, "Document is detached from window."));
+
+        WebPushClient* client = PushController::clientFrom(document->frame());
+        ASSERT(client);
+        RefPtr<ScriptPromiseResolver> resolver = ScriptPromiseResolver::create(scriptState);
+        ScriptPromise promise = resolver->promise();

[Peter Beverloo, 2014/12/08 21:40:51]

There's quite a bit of shared code here. Can we perhaps rewrite this method as:

ScriptPromise PushManager::registerPushMessaging(ScriptState* scriptState)
{
    RefPtr<ScriptPromiseResolver> resolver =
ScriptPromiseResolver::create(scriptState);
    ScriptPromise promise = resolver->promise();

    // Different code paths are used for documents and workers because..
[MVANOUWERKERK TO COMPLETE] 
    if (scriptState->executionContext()->isDocument()) {
        Document* document = toDocument(scriptState->executionContext());
        if (!document->domWindow() || !document->frame())
            return promise.reject(DOMException::create(InvalidStateError,
"Document is detached from window."));

        WebPushClient* client = PushController::clientFrom(document->frame());
        ASSERT(client);

        client->registerPushMessaging(m_registration->webRegistration(), new
CallbackPromiseAdapter<PushRegistration, PushError>(resolver));
    } else {
        pushProvider()->registerPushMessaging(m_registration->webRegistration(),
new CallbackPromiseAdapter<PushRegistration, PushError>(resolver));
    }

    return promise;
}

[Michael van Ouwerkerk, 2014/12/09 18:47:45]

Done.

+        client->registerPushMessaging(m_registration->webRegistration(), new CallbackPromiseAdapter<PushRegistration, PushError>(resolver));
+        return promise;
+    }
 
     RefPtr<ScriptPromiseResolver> resolver = ScriptPromiseResolver::create(scriptState);
     ScriptPromise promise = resolver->promise();
-
-    client->registerPushMessaging(new CallbackPromiseAdapter<PushRegistration, PushError>(resolver), serviceWorkerProvider);
-
+    Platform::current()->pushProvider()->registerPushMessaging(m_registration->webRegistration(), new CallbackPromiseAdapter<PushRegistration, PushError>(resolver));

[Peter Beverloo, 2014/12/08 21:40:51]

Can you add a utility method to receive the pushProvider()? We'll also want to
ASSERT somewhere to avoid messed up stack traces.

[Michael van Ouwerkerk, 2014/12/09 18:47:45]

Done.

     return promise;
 }
 
-// FIXME: This call should be available from workers which will not have a Document object available.
-// See crbug.com/389194
 ScriptPromise PushManager::hasPermission(ScriptState* scriptState)
 {
-    ASSERT(scriptState->executionContext()->isDocument());
-
-    Document* document = toDocument(scriptState->executionContext());
-    if (!document->domWindow() || !document->frame())
-        return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, "Document is detached from window."));
-    blink::WebPushClient* client = PushController::clientFrom(document->frame());
-    ASSERT(client);
-
-    // The currently implemented specification does not require a Service Worker to be present for the
-    // hasPermission() call to work, but it will become a requirement soon.
-    WebServiceWorkerProvider* serviceWorkerProvider = NavigatorServiceWorker::serviceWorker(*document->domWindow()->navigator())->provider();
-    if (!serviceWorkerProvider)
-        return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, "No Service Worker installed for this document."));
+    if (scriptState->executionContext()->isDocument()) {
+        Document* document = toDocument(scriptState->executionContext());
+        if (!document->domWindow() || !document->frame())
+            return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, "Document is detached from window."));

[Peter Beverloo, 2014/12/08 21:40:51]

I'm a bit reluctant to continue having these checks if they're not reproducible
in layout tests. Can we perhaps keep the current layout tests for this, and
update it for the new syntax?

[Michael van Ouwerkerk, 2014/12/09 18:47:45]

As discussed offline, I ran into some trouble porting the detached document
tests using the testharness.js format. I'm happy to give it another shot in
future, I've filed a bug for this: https://crbug.com/440431

I also added an entry to the task tracker and FIXMEs in the code.

+    }
 
     RefPtr<ScriptPromiseResolver> resolver = ScriptPromiseResolver::create(scriptState);
-
     ScriptPromise promise = resolver->promise();
-    client->getPermissionStatus(new PushPermissionStatusCallback(resolver), serviceWorkerProvider);
+    Platform::current()->pushProvider()->getPermissionStatus(m_registration->webRegistration(), new PushPermissionStatusCallback(resolver));
     return promise;
 }
 
+void PushManager::trace(Visitor* visitor)
+{
+    visitor->trace(m_registration);
+}
+
 } // namespace blink