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

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

This part 6 of a series of 6 patches. See https://crbug.com/440076 for the plan to land them.

BUG=389194, 440076
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186978

[Michael van Ouwerkerk, 2014-12-08 20:04:12]

mvanouwerkerk@chromium.org changed reviewers:
+ peter@chromium.org

[Michael van Ouwerkerk, 2014-12-08 20:04:12]

Peter, could you take a look please?

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

Overall I like the direction this is going in, thanks for that! I'm a bit
worried about the loss in test coverage.

We recently introduced layout tests exposing all global objects and their
properties, and I'd be in favor of re-introducing an updated version of
push-messaging-api-surface.html which verifies that the objects and members
exposed in either Document or Worker contexts are exactly as expected.

The second test you're not re-introducing since removing it is to verify that
using push messaging on a detached document does not cause a crash. The checks
in the code are still there, but the tests are not. Is there any way to
reproduce this scenario using a TestHarness test? I'd be OK with continuing to
use non-TestHarness tests for verifying those crashers are gone if that makes it
easier.

https://codereview.chromium.org/783983003/diff/1/LayoutTests/http/tests/push_...
File LayoutTests/http/tests/push_messaging/has-permission-document.html (right):

https://codereview.chromium.org/783983003/diff/1/LayoutTests/http/tests/push_...
LayoutTests/http/tests/push_messaging/has-permission-document.html:38:
.then(function(permissionStatus) {
This clause shouldn't be included, as it tests the TestRunner rather than the
feature.

Instead, we should call testRunner.clearPushMessagingPermissions(); in the test
initialization code to ensure that we start with a clean state.

https://codereview.chromium.org/783983003/diff/1/LayoutTests/http/tests/push_...
File LayoutTests/http/tests/push_messaging/register-document.html (right):

https://codereview.chromium.org/783983003/diff/1/LayoutTests/http/tests/push_...
LayoutTests/http/tests/push_messaging/register-document.html:12:
async_test(function(test) {
This is a bit of a weird test. Also, "register in document context" doesn't
describe what it's doing.

Currently, it:

(1) Unregisters a Service Worker, then registers a new one.
(2) Grants permission for using the Push API.
(3) Registers for Push messaging.
(4) Verifies that the endpoint and registration Id are as defined in the
TestRunner.
(5) Resets existing Push API registrations, revokes permissions.
(6) Registers for Push messaging.
(7) Verifies that an AbortError gets thrown.

That's a bit silly. Firstly, (4) and beyond test the TestRunner rather than the
feature. It's also not obvious what's going on by reading the test.

Instead, I'd propose having a few more focused tests:

(1)
- Registers for Push messaging when permission has been granted.

(2)
- Registers for Push messaging when permission has not been granted.

(3)
- Verify that the endpoint received for a valid Push Registration is a URL.
(Orthogonal of *which* URL.)

(4)
- Verify that registering, then unregistering, then registering again for Push
messaging results in a different push registration Id.


Assuming that (4) is the reason that resetPushMessagingRegistrations() is needed
at all, it may be worthwhile to postpone introducing that method and wait until
the unregister() call works as expected, which really shouldn't be too much
longer.


Think of the TestRunner as tools to instrument your tests, rather than tools
which must be used in your tests.

https://codereview.chromium.org/783983003/diff/1/LayoutTests/http/tests/push_...
File LayoutTests/http/tests/push_messaging/resources/push_manifest.json (right):

https://codereview.chromium.org/783983003/diff/1/LayoutTests/http/tests/push_...
LayoutTests/http/tests/push_messaging/resources/push_manifest.json:2:
"gcm_sender_id": "1234567890"
nit: Four space indent.

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
File Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:16: #include
"core/frame/LocalDOMWindow.h"
nit: This include seems to be unused.

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:21: #include
"modules/serviceworkers/NavigatorServiceWorker.h"
nit: This include seems to be unused.

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:22: #include
"modules/serviceworkers/ServiceWorkerContainer.h"
nit: This include seems to be unused.

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:33: {
ASSERT(registration)

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:46: ScriptPromise promise =
resolver->promise();
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;
}

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:53:
Platform::current()->pushProvider()->registerPushMessaging(m_registration->webRegistration(),
new CallbackPromiseAdapter<PushRegistration, PushError>(resolver));
Can you add a utility method to receive the pushProvider()? We'll also want to
ASSERT somewhere to avoid messed up stack traces.

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:62: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(InvalidStateError, "Document is detached from window."));
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?

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
File Source/modules/push_messaging/ServiceWorkerRegistrationPush.cpp (right):

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/ServiceWorkerRegistrationPush.cpp:53:
HeapSupplement<ServiceWorkerRegistration>::trace(visitor);
HeapSupplement<> should come before the local members.

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
File Source/modules/push_messaging/ServiceWorkerRegistrationPush.idl (right):

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/ServiceWorkerRegistrationPush.idl:4: 
nit: add a link to the spec.

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/ServiceWorkerRegistrationPush.idl:8: readonly
attribute PushManager pushManager;
Should we add a TODO here to mention that the specification currently calls this
pushRegistrationManager, but that we're trying to get it changed?

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

I think this should be a lot better. Please take another look.

https://codereview.chromium.org/783983003/diff/1/LayoutTests/http/tests/push_...
File LayoutTests/http/tests/push_messaging/has-permission-document.html (right):

https://codereview.chromium.org/783983003/diff/1/LayoutTests/http/tests/push_...
LayoutTests/http/tests/push_messaging/has-permission-document.html:38:
.then(function(permissionStatus) {
On 2014/12/08 21:40:50, Peter Beverloo wrote:
> This clause shouldn't be included, as it tests the TestRunner rather than the
> feature.
> 
> Instead, we should call testRunner.clearPushMessagingPermissions(); in the
test
> initialization code to ensure that we start with a clean state.

Done. Actually, I split these into three separate files to avoid concurrency
bugs and make the split clearer.

https://codereview.chromium.org/783983003/diff/1/LayoutTests/http/tests/push_...
File LayoutTests/http/tests/push_messaging/register-document.html (right):

https://codereview.chromium.org/783983003/diff/1/LayoutTests/http/tests/push_...
LayoutTests/http/tests/push_messaging/register-document.html:12:
async_test(function(test) {
On 2014/12/08 21:40:50, Peter Beverloo wrote:
> This is a bit of a weird test. Also, "register in document context" doesn't
> describe what it's doing.
> 
> Currently, it:
> 
> (1) Unregisters a Service Worker, then registers a new one.
> (2) Grants permission for using the Push API.
> (3) Registers for Push messaging.
> (4) Verifies that the endpoint and registration Id are as defined in the
> TestRunner.
> (5) Resets existing Push API registrations, revokes permissions.
> (6) Registers for Push messaging.
> (7) Verifies that an AbortError gets thrown.
> 
> That's a bit silly. Firstly, (4) and beyond test the TestRunner rather than
the
> feature. It's also not obvious what's going on by reading the test.
> 
> Instead, I'd propose having a few more focused tests:
> 
> (1)
> - Registers for Push messaging when permission has been granted.
> 
> (2)
> - Registers for Push messaging when permission has not been granted.
> 
> (3)
> - Verify that the endpoint received for a valid Push Registration is a URL.
> (Orthogonal of *which* URL.)
> 
> (4)
> - Verify that registering, then unregistering, then registering again for Push
> messaging results in a different push registration Id.
> 
> 
> Assuming that (4) is the reason that resetPushMessagingRegistrations() is
needed
> at all, it may be worthwhile to postpone introducing that method and wait
until
> the unregister() call works as expected, which really shouldn't be too much
> longer.
> 
> 
> Think of the TestRunner as tools to instrument your tests, rather than tools
> which must be used in your tests.

It was mixed up because multiple async tests on the same page don't work
correctly with service workers and permissions management. I've split the
following into separate test pages now:
* register success
* register failure due to permission denied
* register failure due to worker not being active yet

The relevant API surfaces are verified in there as well, doing that separately
would duplicate an awful lot of code.

It does seem fine to wait with (4) until unregister() and getRegistration() have
landed, so we can avoid unnecessary TestRunner methods.

https://codereview.chromium.org/783983003/diff/1/LayoutTests/http/tests/push_...
File LayoutTests/http/tests/push_messaging/resources/push_manifest.json (right):

https://codereview.chromium.org/783983003/diff/1/LayoutTests/http/tests/push_...
LayoutTests/http/tests/push_messaging/resources/push_manifest.json:2:
"gcm_sender_id": "1234567890"
On 2014/12/08 21:40:51, Peter Beverloo wrote:
> nit: Four space indent.

Done.

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
File Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:16: #include
"core/frame/LocalDOMWindow.h"
On 2014/12/08 21:40:51, Peter Beverloo wrote:
> nit: This include seems to be unused.

Done.

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:21: #include
"modules/serviceworkers/NavigatorServiceWorker.h"
On 2014/12/08 21:40:51, Peter Beverloo wrote:
> nit: This include seems to be unused.

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

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:22: #include
"modules/serviceworkers/ServiceWorkerContainer.h"
On 2014/12/08 21:40:51, Peter Beverloo wrote:
> nit: This include seems to be unused.

Done.

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:33: {
On 2014/12/08 21:40:51, Peter Beverloo wrote:
> ASSERT(registration)

Done.

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:46: ScriptPromise promise =
resolver->promise();
On 2014/12/08 21:40:51, Peter Beverloo wrote:
> 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;
> }

Done.

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:53:
Platform::current()->pushProvider()->registerPushMessaging(m_registration->webRegistration(),
new CallbackPromiseAdapter<PushRegistration, PushError>(resolver));
On 2014/12/08 21:40:51, Peter Beverloo wrote:
> Can you add a utility method to receive the pushProvider()? We'll also want to
> ASSERT somewhere to avoid messed up stack traces.

Done.

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:62: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(InvalidStateError, "Document is detached from window."));
On 2014/12/08 21:40:51, Peter Beverloo wrote:
> 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?

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.

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
File Source/modules/push_messaging/ServiceWorkerRegistrationPush.cpp (right):

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/ServiceWorkerRegistrationPush.cpp:53:
HeapSupplement<ServiceWorkerRegistration>::trace(visitor);
On 2014/12/08 21:40:51, Peter Beverloo wrote:
> HeapSupplement<> should come before the local members.

All existing code seems to do the opposite, local members go first. I could find
no examples of what you described. Sometimes other parent calls go after though.
Who would know the finer details about this? I thought of pinging Sigbjorn but
wasn't sure about pinging an Opera member.

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
File Source/modules/push_messaging/ServiceWorkerRegistrationPush.idl (right):

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/ServiceWorkerRegistrationPush.idl:4: 
On 2014/12/08 21:40:51, Peter Beverloo wrote:
> nit: add a link to the spec.

Done.

https://codereview.chromium.org/783983003/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/ServiceWorkerRegistrationPush.idl:8: readonly
attribute PushManager pushManager;
On 2014/12/08 21:40:51, Peter Beverloo wrote:
> Should we add a TODO here to mention that the specification currently calls
this
> pushRegistrationManager, but that we're trying to get it changed?

Done.

[Peter Beverloo, 2014-12-10 00:18:18]

lgtm

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/push_messaging/has_permission_default.html (right):

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
LayoutTests/http/tests/push_messaging/has_permission_default.html:4: <title>Push
API: hasPermission fulfills with default when no permission is set</title>
nit: s/fulfills/resolves/ (In the test's description too.)

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/push_messaging/has_permission_denied.html (right):

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
LayoutTests/http/tests/push_messaging/has_permission_denied.html:4: <title>Push
API: hasPermission fulfills with denied when permission is denied</title>
nit: s/fulfills/resolves/ (In the test's description too.)

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
LayoutTests/http/tests/push_messaging/has_permission_denied.html:13:
assert_true(!!window.testRunner, 'This test requires the TestRunner.');
It might be nicer to make the testRunner "optional", and adding a JavaScript
comment that the person running the test should reject Push permission and
reload the page. It's the small things that count...

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/push_messaging/has_permission_granted.html (right):

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
LayoutTests/http/tests/push_messaging/has_permission_granted.html:4: <title>Push
API: hasPermission fulfills with granted when permission is granted</title>
nit: s/fulfills/resolves/ (In the test's description too.)

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/push_messaging/register_success.html (right):

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
LayoutTests/http/tests/push_messaging/register_success.html:4: <title>register()
succeeds when permission is granted and fulfills with a valid
registration</title>
nit: s/fulfills/resolves/ (In the test's description too.)

And everywhere else, to avoid repeating myself even more.

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/push_messaging/register_worker_not_activated.html
(right):

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
LayoutTests/http/tests/push_messaging/register_worker_not_activated.html:20:
assert_true(!!swRegistration.installing, 'The worker should be installing');
assert_not_equals(swRegistration.installing, null)? I find that more readable
than !!

https://codereview.chromium.org/783983003/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/783983003/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.cpp:29: WebPushProvider*
pushProvider()
nit: no indentation.

https://codereview.chromium.org/783983003/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.cpp:47: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(AbortError, "Registration failed - no Service Worker"));
Please include a better error message here. The developer may not be aware of
the differences between an installing or active service worker initially, and
this doesn't help them.

https://codereview.chromium.org/783983003/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.cpp:61: WebPushClient* client =
PushController::clientFrom(document->frame());
If we're asserting on every use anyway, we may as well ASSERT in
PushController::clientFrom and return a reference instead.

https://codereview.chromium.org/783983003/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/ServiceWorkerRegistrationPush.idl (right):

https://codereview.chromium.org/783983003/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/ServiceWorkerRegistrationPush.idl:10: // FIXME:
this may be renamed depending on the result of
https://github.com/w3c/push-api/issues/47
This FIXME feels a bit useless. If we rename it we'll send a patch, if we
don't... we don't have to send a patch. Not we have to send a patch for either
outcome.

[Michael van Ouwerkerk, 2014-12-10 15:13:06]

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/push_messaging/has_permission_default.html (right):

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
LayoutTests/http/tests/push_messaging/has_permission_default.html:4: <title>Push
API: hasPermission fulfills with default when no permission is set</title>
On 2014/12/10 00:18:17, Peter Beverloo wrote:
> nit: s/fulfills/resolves/ (In the test's description too.)

Done.

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/push_messaging/has_permission_denied.html (right):

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
LayoutTests/http/tests/push_messaging/has_permission_denied.html:4: <title>Push
API: hasPermission fulfills with denied when permission is denied</title>
On 2014/12/10 00:18:17, Peter Beverloo wrote:
> nit: s/fulfills/resolves/ (In the test's description too.)

Done.

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
LayoutTests/http/tests/push_messaging/has_permission_denied.html:13:
assert_true(!!window.testRunner, 'This test requires the TestRunner.');
On 2014/12/10 00:18:17, Peter Beverloo wrote:
> It might be nicer to make the testRunner "optional", and adding a JavaScript
> comment that the person running the test should reject Push permission and
> reload the page. It's the small things that count...

Done.

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/push_messaging/has_permission_granted.html (right):

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
LayoutTests/http/tests/push_messaging/has_permission_granted.html:4: <title>Push
API: hasPermission fulfills with granted when permission is granted</title>
On 2014/12/10 00:18:17, Peter Beverloo wrote:
> nit: s/fulfills/resolves/ (In the test's description too.)

Done.

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/push_messaging/register_success.html (right):

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
LayoutTests/http/tests/push_messaging/register_success.html:4: <title>register()
succeeds when permission is granted and fulfills with a valid
registration</title>
On 2014/12/10 00:18:18, Peter Beverloo wrote:
> nit: s/fulfills/resolves/ (In the test's description too.)
> 
> And everywhere else, to avoid repeating myself even more.

Done.

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/push_messaging/register_worker_not_activated.html
(right):

https://codereview.chromium.org/783983003/diff/20001/LayoutTests/http/tests/p...
LayoutTests/http/tests/push_messaging/register_worker_not_activated.html:20:
assert_true(!!swRegistration.installing, 'The worker should be installing');
On 2014/12/10 00:18:18, Peter Beverloo wrote:
> assert_not_equals(swRegistration.installing, null)? I find that more readable
> than !!

Done.

https://codereview.chromium.org/783983003/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/783983003/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.cpp:29: WebPushProvider*
pushProvider()
On 2014/12/10 00:18:18, Peter Beverloo wrote:
> nit: no indentation.

Done.

https://codereview.chromium.org/783983003/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.cpp:47: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(AbortError, "Registration failed - no Service Worker"));
On 2014/12/10 00:18:18, Peter Beverloo wrote:
> Please include a better error message here. The developer may not be aware of
> the differences between an installing or active service worker initially, and
> this doesn't help them.

Done.

https://codereview.chromium.org/783983003/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.cpp:61: WebPushClient* client =
PushController::clientFrom(document->frame());
On 2014/12/10 00:18:18, Peter Beverloo wrote:
> If we're asserting on every use anyway, we may as well ASSERT in
> PushController::clientFrom and return a reference instead.

Done.

https://codereview.chromium.org/783983003/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/ServiceWorkerRegistrationPush.idl (right):

https://codereview.chromium.org/783983003/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/ServiceWorkerRegistrationPush.idl:10: // FIXME:
this may be renamed depending on the result of
https://github.com/w3c/push-api/issues/47
On 2014/12/10 00:18:18, Peter Beverloo wrote:
> This FIXME feels a bit useless. If we rename it we'll send a patch, if we
> don't... we don't have to send a patch. Not we have to send a patch for either
> outcome.

You asked for a TODO here. Are you sure about this? I'm not sure I understand
what you're looking for here. Either way, it's trivial, but I'd prefer not to
have such a comment as the consensus seems to be to use pushManager of type
PushManager.

[Michael van Ouwerkerk, 2014-12-11 16:50:38]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2014-12-11 16:51:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/783983003/40001

[commit-bot: I haz the power, 2014-12-11 18:28:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-11 18:28:45]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/40940)

[Michael van Ouwerkerk, 2014-12-11 18:30:13]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2014-12-11 18:31:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/783983003/40001

[commit-bot: I haz the power, 2014-12-11 19:55:35]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186978