Actually hook up ServiceWorkerRegistration.showNotification()

Actually hook up ServiceWorkerRegistration.showNotification()

Rather than always returning a rejected promise, after this patch we
defer to content/ for displaying the notification. Since the layout
test runner has been updated, we can actually start testing Web
Notifications shown using SWR.showNotification() now.

BUG=432527
R=mvanouwerkerk@chromium.org, yhirano@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187026

[Peter Beverloo, 2014-12-09 15:59:18]

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

[Peter Beverloo, 2014-12-09 15:59:19]

+mvanouwerkerk for review

This depends on https://codereview.chromium.org/789523002/.

[Michael van Ouwerkerk, 2014-12-09 19:38:04]

lgtm with nits and clarifying questions

https://codereview.chromium.org/787893002/diff/20001/LayoutTests/http/tests/n...
File
LayoutTests/http/tests/notifications/service-worker-show-notification-click.html
(right):

https://codereview.chromium.org/787893002/diff/20001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/service-worker-show-notification-click.html:16:
worker_url = 'resources/click-forward-service-worker.js';
nit: workerUrl

https://codereview.chromium.org/787893002/diff/20001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/service-worker-show-notification-click.html:18:
testRunner.grantWebNotificationPermission("http://127.0.0.1:8000", true);
Nit: just location.origin should be enough

https://codereview.chromium.org/787893002/diff/20001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/service-worker-show-notification-click.html:24:
return wait_for_update(test, registration);
I don't think you need this if you use wait_for_activated

https://codereview.chromium.org/787893002/diff/20001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/service-worker-show-notification-click.html:26:
return wait_for_state(test, serviceWorker, 'activating');
nit: return wait_for_activated(test, registration);

https://codereview.chromium.org/787893002/diff/20001/Source/modules/notificat...
File Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/787893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:24:
ScriptPromise
ServiceWorkerRegistrationNotifications::showNotification(ScriptState*
scriptState, ServiceWorkerRegistration& serviceWorkerRegistration, const String&
title, const NotificationOptions& options)
Why can't ServiceWorkerRegistration& be const?

https://codereview.chromium.org/787893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:36:
RefPtr<ScriptPromiseResolver> resolver =
ScriptPromiseResolver::create(scriptState);
Might want to keep an eye on this patch and resolve any conflict if it lands
first: https://codereview.chromium.org/783423003/

https://codereview.chromium.org/787893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:52:
SecurityOrigin* origin = executionContext->securityOrigin();
It would seem you don't need to pass the origin along, since you can read it
from the ServiceWorkerRegistration. No big deal though.

[Peter Beverloo, 2014-12-10 13:24:35]

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

[Peter Beverloo, 2014-12-10 13:24:36]

+mkwst

https://codereview.chromium.org/787893002/diff/20001/LayoutTests/http/tests/n...
File
LayoutTests/http/tests/notifications/service-worker-show-notification-click.html
(right):

https://codereview.chromium.org/787893002/diff/20001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/service-worker-show-notification-click.html:16:
worker_url = 'resources/click-forward-service-worker.js';
On 2014/12/09 19:38:04, Michael van Ouwerkerk wrote:
> nit: workerUrl

Done.

https://codereview.chromium.org/787893002/diff/20001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/service-worker-show-notification-click.html:18:
testRunner.grantWebNotificationPermission("http://127.0.0.1:8000", true);
On 2014/12/09 19:38:04, Michael van Ouwerkerk wrote:
> Nit: just location.origin should be enough

Done.

https://codereview.chromium.org/787893002/diff/20001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/service-worker-show-notification-click.html:24:
return wait_for_update(test, registration);
On 2014/12/09 19:38:04, Michael van Ouwerkerk wrote:
> I don't think you need this if you use wait_for_activated

Done.

https://codereview.chromium.org/787893002/diff/20001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/service-worker-show-notification-click.html:26:
return wait_for_state(test, serviceWorker, 'activating');
On 2014/12/09 19:38:04, Michael van Ouwerkerk wrote:
> nit: return wait_for_activated(test, registration);

Cool, thanks! You do want to take note of
https://codereview.chromium.org/790683002 however. Done.

https://codereview.chromium.org/787893002/diff/20001/Source/modules/notificat...
File Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/787893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:24:
ScriptPromise
ServiceWorkerRegistrationNotifications::showNotification(ScriptState*
scriptState, ServiceWorkerRegistration& serviceWorkerRegistration, const String&
title, const NotificationOptions& options)
On 2014/12/09 19:38:04, Michael van Ouwerkerk wrote:
> Why can't ServiceWorkerRegistration& be const?

Neither .active() nor .webRegistration() are defined as being const, or return
const types.

https://codereview.chromium.org/787893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:36:
RefPtr<ScriptPromiseResolver> resolver =
ScriptPromiseResolver::create(scriptState);
On 2014/12/09 19:38:04, Michael van Ouwerkerk wrote:
> Might want to keep an eye on this patch and resolve any conflict if it lands
> first: https://codereview.chromium.org/783423003/

Will do, thanks for the pointer!

https://codereview.chromium.org/787893002/diff/20001/Source/modules/notificat...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:52:
SecurityOrigin* origin = executionContext->securityOrigin();
On 2014/12/09 19:38:04, Michael van Ouwerkerk wrote:
> It would seem you don't need to pass the origin along, since you can read it
> from the ServiceWorkerRegistration. No big deal though.

Acknowledged. I'll opt for more three-sided clean-up patches later :-).

[Peter Beverloo, 2014-12-10 19:38:05]

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

[Peter Beverloo, 2014-12-10 19:38:05]

+dglazkov for review (CallbackPromiseAdapter.h stamps)

[dglazkov, 2014-12-10 19:57:37]

dglazkov@chromium.org changed reviewers:
+ jochen@chromium.org

[dglazkov, 2014-12-10 19:57:51]

+jochen

[Peter Beverloo, 2014-12-11 19:06:25]

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

[Peter Beverloo, 2014-12-11 19:06:26]

+adamk for Source/bindings/

[adamk, 2014-12-11 20:57:14]

adamk@chromium.org changed reviewers:
+ yhirano@chromium.org
- adamk@chromium.org, dglazkov@chromium.org, jochen@chromium.org

[adamk, 2014-12-11 20:57:14]

Sorry to bounce you one more time, but yhirano is the right reviewer for this,
being Promise-related as well as bindings.

[yhirano, 2014-12-12 01:34:37]

bindings/ change lgtm

[Peter Beverloo, 2014-12-12 11:05:12]

Thanks!

[Peter Beverloo, 2014-12-12 11:05:15]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-12-12 11:05:58]

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

[Peter Beverloo, 2014-12-12 13:57:28]

The CQ bit was unchecked by peter@chromium.org

[Peter Beverloo, 2014-12-12 13:57:45]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-12-12 13:59:03]

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

[Peter Beverloo, 2014-12-12 14:07:06]

The CQ bit was unchecked by peter@chromium.org

[Peter Beverloo, 2014-12-12 14:12:25]

Committed patchset #3 (id:40001) manually as 187026 (presubmit successful).