Support persistent notifications in the PlatformNotificationServiceImpl.

Support persistent notifications in the PlatformNotificationServiceImpl.

This patch makes it possible to create and use persistent notifications based
on delegates. This works reliable on desktop, but does not yet implement the
after-browser-restart call for Android.

BUG=432527
Committed: https://crrev.com/eb322fc9a5882550d2a34b04954a5b3f9f8e63f9
Cr-Commit-Position: refs/heads/master@{#308353}

[Peter Beverloo, 2014-12-09 19:43:53]

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

[Peter Beverloo, 2014-12-09 19:43:54]

Together with the following three patches, this provides a full, end-to-end
implementation of persistent notifications.

In order of dependencies:
  (1) https://codereview.chromium.org/774573003/
  (2) https://codereview.chromium.org/787193002/
  (3) https://codereview.chromium.org/776813004/

This works for all platforms, including Android, except in the case where the
browser died completely. That will be handled in a separate patch.

PTAL

[dewittj, 2014-12-10 00:35:04]

where is NotificationEventDispatcher defined?

[Peter Beverloo, 2014-12-10 00:41:16]

On 2014/12/10 00:35:04, dewittj wrote:
> where is NotificationEventDispatcher defined?

content/public/browser/notification_event_dispatcher.h.
Implemented by
content/browser/notifications/notification_event_dispatcher_impl.cc

[dewittj, 2014-12-10 01:01:31]

hm, not showing up in code search or
http://git.chromium.org/gitweb/?p=chromium/src.git;a=tree;f=content/browser;h...

is it not yet landed?

[Peter Beverloo, 2014-12-10 22:00:16]

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

[Peter Beverloo, 2014-12-10 22:00:17]

Justin: PTAL.

I added both unit tests and browser tests to this CL, and amended the
description to accurately describe what it does. It should be ready to go from a
Notifications POV.

I can't expect you to know (or learn) all the ins and outs about Service
Workers, their states and how this interacts with persistent notifications.
mvanouwerkerk@ - would you mind reviewing the browser test from a technical POV?

[Peter Beverloo, 2014-12-10 22:01:57]

To clarify, this CL depends on:
  https://codereview.chromium.org/787193002/ (moving logic to PNS)
  https://codereview.chromium.org/787893002/ (hook up the Blink API)
  https://codereview.chromium.org/789643003/ (SW event completion)

[Michael van Ouwerkerk, 2014-12-11 16:41:55]

lgtm but it would be good to address at least some of these nits and questions

https://codereview.chromium.org/784383002/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/platform_notification_service_browsertest.cc
(right):

https://codereview.chromium.org/784383002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/platform_notification_service_browsertest.cc:27: //
responsed to asynchronously, to immitate the behavior of a user.
nit: imitate

https://codereview.chromium.org/784383002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/platform_notification_service_browsertest.cc:34: 
nit: blank line --

https://codereview.chromium.org/784383002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/platform_notification_service_browsertest.cc:75: if
(accept_) {
nit: wanna omit the curly braces?

https://codereview.chromium.org/784383002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/platform_notification_service_browsertest.cc:107:
// will be returned, indicating whether the JavaScript could be executed.
nit: s/could be executed/was executed successfully/

https://codereview.chromium.org/784383002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/platform_notification_service_browsertest.cc:134:
https_server_.reset(new net::SpawnedTestServer(
So I'm no longer sure we need an https server, as localhost should be considered
secure. I struggled with a security exception, but I think in the end it was
really about the mime type with which the service worker was served.

https://codereview.chromium.org/784383002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/platform_notification_service_browsertest.cc:155:
ui_manager_.reset();
I'm not sure I understand this correctly. These are both scoped_ptr, why do you
need to manually reset them?

https://codereview.chromium.org/784383002/diff/40001/chrome/test/data/notific...
File chrome/test/data/notifications/platform_notification_service.html (right):

https://codereview.chromium.org/784383002/diff/40001/chrome/test/data/notific...
chrome/test/data/notifications/platform_notification_service.html:26: function
GetActivatedServiceWorker(script, scope) {
This is nice, it would be useful to generalize as well.

https://codereview.chromium.org/784383002/diff/40001/chrome/test/data/notific...
chrome/test/data/notifications/platform_notification_service.html:69:
GetActivatedServiceWorker('platform_notification_service.js',
nit: it would be clearer if this js file name had the words 'service worker' in
them.

https://codereview.chromium.org/784383002/diff/40001/chrome/test/data/notific...
chrome/test/data/notifications/platform_notification_service.html:101:
domAutomationController.send('' + messageQueue.pop());
Any particular reason why this is acting like a stack i.e. first in, last out?
Might as well call it a stack then.

[dewittj, 2014-12-11 17:18:50]

https://codereview.chromium.org/784383002/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/persistent_notification_delegate.cc (right):

https://codereview.chromium.org/784383002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/persistent_notification_delegate.cc:27:
id_(base::GenerateGUID()) {}
If I read this correctly, one of the use cases for persistent notification
delegates is to create them out of thin air based on information from the
Android notification center.  In that case, does it make sense to use a GUID for
the ID, since then a recreated notification will have a different ID than the
previous one.

It looks like we never use the ID over in service worker land:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

but with the TODO there using a random ID here seems a bit risky.

https://codereview.chromium.org/784383002/diff/40001/chrome/test/data/notific...
File chrome/test/data/notifications/platform_notification_service.html (right):

https://codereview.chromium.org/784383002/diff/40001/chrome/test/data/notific...
chrome/test/data/notifications/platform_notification_service.html:37: return
Promise.resolve(registration);
If I recall correctly you can just return registration.

[Peter Beverloo, 2014-12-11 19:02:00]

Thanks for the comments! PTAL

https://codereview.chromium.org/784383002/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/persistent_notification_delegate.cc (right):

https://codereview.chromium.org/784383002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/persistent_notification_delegate.cc:27:
id_(base::GenerateGUID()) {}
On 2014/12/11 17:18:50, dewittj wrote:
> If I read this correctly, one of the use cases for persistent notification
> delegates is to create them out of thin air based on information from the
> Android notification center.  In that case, does it make sense to use a GUID
for
> the ID, since then a recreated notification will have a different ID than the
> previous one.
> 
> It looks like we never use the ID over in service worker land:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> but with the TODO there using a random ID here seems a bit risky.

I'm not sure whether it makes sense to re-create a
PersistentNotificationDelegate object from the NotificationUIManagerAndroid. My
current patch invokes PNSImpl::OnPersistentNotificationClick directly from the
NotifiationUIManagerAndroid. That also gives Android the ability to provide its
own DispatchComplete callback.

https://codereview.chromium.org/784383002/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/platform_notification_service_browsertest.cc
(right):

https://codereview.chromium.org/784383002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/platform_notification_service_browsertest.cc:27: //
responsed to asynchronously, to immitate the behavior of a user.
On 2014/12/11 16:41:55, Michael van Ouwerkerk wrote:
> nit: imitate

Done.

https://codereview.chromium.org/784383002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/platform_notification_service_browsertest.cc:34: 
On 2014/12/11 16:41:55, Michael van Ouwerkerk wrote:
> nit: blank line --

Done.

https://codereview.chromium.org/784383002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/platform_notification_service_browsertest.cc:75: if
(accept_) {
On 2014/12/11 16:41:55, Michael van Ouwerkerk wrote:
> nit: wanna omit the curly braces?

Done.

https://codereview.chromium.org/784383002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/platform_notification_service_browsertest.cc:107:
// will be returned, indicating whether the JavaScript could be executed.
On 2014/12/11 16:41:55, Michael van Ouwerkerk wrote:
> nit: s/could be executed/was executed successfully/

Done.

https://codereview.chromium.org/784383002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/platform_notification_service_browsertest.cc:134:
https_server_.reset(new net::SpawnedTestServer(
On 2014/12/11 16:41:55, Michael van Ouwerkerk wrote:
> So I'm no longer sure we need an https server, as localhost should be
considered
> secure. I struggled with a security exception, but I think in the end it was
> really about the mime type with which the service worker was served.

Technically we don't, the code works perfectly fine when we use a HTTP server.
(127.0.0.1 indeed is recognized as a secure origin.)

However, with the world moving the HTTPS, I'm not sure whether using HTTP over
HTTPS really makes sense. The overhead of HTTPS not measurable, so I'd rather
keep it as-is right now.

https://codereview.chromium.org/784383002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/platform_notification_service_browsertest.cc:155:
ui_manager_.reset();
On 2014/12/11 16:41:55, Michael van Ouwerkerk wrote:
> I'm not sure I understand this correctly. These are both scoped_ptr, why do
you
> need to manually reset them?

Precedence in a certain other browser test :-). I removed them, as there's no
strict need, and they mainly satisfied my mental image about cleaning up after
oneself.

https://codereview.chromium.org/784383002/diff/40001/chrome/test/data/notific...
File chrome/test/data/notifications/platform_notification_service.html (right):

https://codereview.chromium.org/784383002/diff/40001/chrome/test/data/notific...
chrome/test/data/notifications/platform_notification_service.html:26: function
GetActivatedServiceWorker(script, scope) {
On 2014/12/11 16:41:55, Michael van Ouwerkerk wrote:
> This is nice, it would be useful to generalize as well.

Added a TODO.

https://codereview.chromium.org/784383002/diff/40001/chrome/test/data/notific...
chrome/test/data/notifications/platform_notification_service.html:37: return
Promise.resolve(registration);
On 2014/12/11 17:18:50, dewittj wrote:
> If I recall correctly you can just return registration.

Done.

https://codereview.chromium.org/784383002/diff/40001/chrome/test/data/notific...
chrome/test/data/notifications/platform_notification_service.html:69:
GetActivatedServiceWorker('platform_notification_service.js',
On 2014/12/11 16:41:55, Michael van Ouwerkerk wrote:
> nit: it would be clearer if this js file name had the words 'service worker'
in
> them.

Ack. I don't have a strong preference of one over the other, but
platform_notification_service_service_worker.js would cause all kinds of funky
wrapping. Do you feel strongly about this? The comment in the .js file clearly
explains what it's for.

https://codereview.chromium.org/784383002/diff/40001/chrome/test/data/notific...
chrome/test/data/notifications/platform_notification_service.html:101:
domAutomationController.send('' + messageQueue.pop());
On 2014/12/11 16:41:55, Michael van Ouwerkerk wrote:
> Any particular reason why this is acting like a stack i.e. first in, last out?
> Might as well call it a stack then.

Done.

[Michael van Ouwerkerk, 2014-12-11 20:01:19]

still lgtm, thanks for the changes

https://codereview.chromium.org/784383002/diff/40001/chrome/test/data/notific...
File chrome/test/data/notifications/platform_notification_service.html (right):

https://codereview.chromium.org/784383002/diff/40001/chrome/test/data/notific...
chrome/test/data/notifications/platform_notification_service.html:69:
GetActivatedServiceWorker('platform_notification_service.js',
On 2014/12/11 19:02:00, Peter Beverloo wrote:
> On 2014/12/11 16:41:55, Michael van Ouwerkerk wrote:
> > nit: it would be clearer if this js file name had the words 'service worker'
> in
> > them.
> 
> Ack. I don't have a strong preference of one over the other, but
> platform_notification_service_service_worker.js would cause all kinds of funky
> wrapping. Do you feel strongly about this? The comment in the .js file clearly
> explains what it's for.

As I said, it's a readability nit. Don't do it if you feel it's not quite right,
I don't feel strongly about it.

[dewittj, 2014-12-11 20:53:44]

okay, thanks for the explanation.

lgtm

[Peter Beverloo, 2014-12-13 16:17:28]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-12-13 16:17:39]

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

[commit-bot: I haz the power, 2014-12-13 16:20:48]

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

[commit-bot: I haz the power, 2014-12-13 16:20:49]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Peter Beverloo, 2014-12-15 14:06:58]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-12-15 14:07:31]

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

[commit-bot: I haz the power, 2014-12-15 14:15:52]

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

[commit-bot: I haz the power, 2014-12-15 14:15:52]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Peter Beverloo, 2014-12-15 14:33:24]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-12-15 14:34:00]

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

[commit-bot: I haz the power, 2014-12-15 15:31:02]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-15 15:32:44]

Patchset 6 (id:??) landed as
https://crrev.com/eb322fc9a5882550d2a34b04954a5b3f9f8e63f9
Cr-Commit-Position: refs/heads/master@{#308353}