[WebAPKs] Plumb service worker scope to notifications

This CL plumbs the service worker scope to
PlatformNotificationServiceImpl::DisplayPersistentNotification()

The service worker scope is used to determine whether the notification
should be displayed using a WebAPK as the source or using Chrome as the
source. On Android, the notification source is displayed when a user
long-presses on a notification.

A WebAPK is used as the source of the notification if the "service
worker scope" falls within the "WebAPK scope". The "WebAPK scope"
is determined by the "scope" attribute in the WebAPK's Web Manifest
(http://html5doctor.com/web-manifest-specification/)

BUG=627842
TEST=PlatformNotificationServiceBrowserTest.PersistentNotificationServiceWorkerScope

Committed: https://crrev.com/1f71208bc7377d187085071782ca34c6de8e2c1e
Cr-Commit-Position: refs/heads/master@{#406570}

[pkotwicz, 2016-07-14 19:09:41]

Description was changed from

==========
Review-Url: https://codereview.chromium.org/2132063002
Cr-Commit-Position: refs/heads/master@{#404910}
==========

to

==========
This CL plumbs the service worker scope to 
PlatformNotificationServiceImpl::DisplayPersistentNotification()

The service worker scope is used to determine whether the notification
should be displayed using a WebAPK as the source or using Chrome as the
source. On Android, the notification source is displayed when a user
long-presses on a notification.

A WebAPK is used as the source of the notification if the "service
worker scope" falls within the "WebAPK scope". The "WebAPK scope"
is determined by the "scope" attribute in the WebAPK's Web Manifest
(http://html5doctor.com/web-manifest-specification/)

BUG=627842
==========

[pkotwicz, 2016-07-14 19:09:41]

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

[pkotwicz, 2016-07-14 19:22:19]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2016-07-14 19:32:51]

Peter Beverloo can you please take a look? This is an initial CL. I do not know
if I am on the right track. I know that I need to add tests. I will add tests
once the architecture of this CL is approved.

https://codereview.chromium.org/2151993002/diff/20001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/2151993002/diff/20001/chrome/browser/DEPS#new...
chrome/browser/DEPS:15: "+content/common/service_worker",
I suspect that I need to add ServiceWorkerContext::FindReadyRegistrationForId()

ServiceWorkerRegistration is not in content/public/browser though...

https://codereview.chromium.org/2151993002/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_notification_manager.cc
(right):

https://codereview.chromium.org/2151993002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_notification_manager.cc:366:
&content::ServiceWorkerContextWrapper::FindReadyRegistrationForId,
Is it safe to call FindReadyRegistrationForId()? The method seems to have side
effects, notably calling
ServiceWorkerRegistration::ActivateWaitingVersionWhenReady()

I do not know what ServiceWorkerRegistration::ActivateWaitingVersionWhenReady()
does.

[Peter Beverloo, 2016-07-15 13:40:11]

https://codereview.chromium.org/2151993002/diff/20001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/2151993002/diff/20001/chrome/browser/DEPS#new...
chrome/browser/DEPS:15: "+content/common/service_worker",
On 2016/07/14 19:32:50, pkotwicz wrote:
> I suspect that I need to add
ServiceWorkerContext::FindReadyRegistrationForId()
> 
> ServiceWorkerRegistration is not in content/public/browser though...

Sorry, we can't add these dependencies. See the documentation at:
  https://www.chromium.org/developers/content-module/content-api

The general gist is that anything using //content must include files from
the public API in //content/public/. As you spotted, SWR is not part of
that.

I'll suggest an alternative where you use these.

https://codereview.chromium.org/2151993002/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/2151993002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:292:
CreateNotificationFromData(profile, GURL(), origin, notification_data,
nit: GURL() -> GURL() /* service_worker_scope */

(elsewhere too)

https://codereview.chromium.org/2151993002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:424: const
GURL& scope,
general nit: naming for this value is all over the place in your CL :-) Would
you mind settling on one name (service_worker_scope has my preference) and using
it everywhere?

https://codereview.chromium.org/2151993002/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_notification_manager.cc
(right):

https://codereview.chromium.org/2151993002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_notification_manager.cc:359: // Get
the service worker scope.
The //content API does not allow us to do this (see my comment for the DEPS
file), so we have three options:

  (1) Expose the necessary functionality in the //content API;
  (2) Expose a more generic //content API for displaying a notification;
  (3) Don't do anything in this case.

We reach this point in the code when the website didn't show a notification by
themselves, despite being required to. This is an exceptional case, and
developers go through a lot of effort to prevent it. UMA agrees with these
statements :-).

As such, my suggestion would be to go for option (3)- revert the changes in this
file, and pass the origin instead of the Service Worker's scope. The consequence
would be that, in a small percentage of this already exceptional case, we'd
attribute the notification to Chrome as opposed to the WebAPK. I think that's
OK.

One final thing to consider here are users blocking notifications for the
WebAPK, but not for Chrome- it might show up after all... My gut feel is that we
should have UMA evidence for this case before doing //content API surgery.

https://codereview.chromium.org/2151993002/diff/20001/content/browser/notific...
File content/browser/notifications/notification_message_filter.cc (right):

https://codereview.chromium.org/2151993002/diff/20001/content/browser/notific...
content/browser/notifications/notification_message_filter.cc:156: const
PlatformNotificationHostMsg_ShowPersistent_Params& params) {
I don't think you have to make this change.

Instead, you can pass the NotificationMessageFilter a reference to the
ServiceWorkerContext when instantiating it in the RPH
(StoragePartitionImpl::GetServiceWorkerContext()) and call
FindReadyRegistrationForId() from there.

[pkotwicz, 2016-07-15 19:47:02]

Patchset #2 (id:40001) has been deleted

[pkotwicz, 2016-07-15 19:47:16]

Description was changed from

==========
This CL plumbs the service worker scope to 
PlatformNotificationServiceImpl::DisplayPersistentNotification()

The service worker scope is used to determine whether the notification
should be displayed using a WebAPK as the source or using Chrome as the
source. On Android, the notification source is displayed when a user
long-presses on a notification.

A WebAPK is used as the source of the notification if the "service
worker scope" falls within the "WebAPK scope". The "WebAPK scope"
is determined by the "scope" attribute in the WebAPK's Web Manifest
(http://html5doctor.com/web-manifest-specification/)

BUG=627842
==========

to

==========
This CL plumbs the service worker scope to 
PlatformNotificationServiceImpl::DisplayPersistentNotification()

The service worker scope is used to determine whether the notification
should be displayed using a WebAPK as the source or using Chrome as the
source. On Android, the notification source is displayed when a user
long-presses on a notification.

A WebAPK is used as the source of the notification if the "service
worker scope" falls within the "WebAPK scope". The "WebAPK scope"
is determined by the "scope" attribute in the WebAPK's Web Manifest
(http://html5doctor.com/web-manifest-specification/)

BUG=627842
TEST=PlatformNotificationServiceBrowserTest.PersistentNotificationServiceWorkerScope
==========

[pkotwicz, 2016-07-15 19:48:45]

Peter Beverloo can you please take a look? I think that I have addressed all of
your comments. I have also added a test

https://codereview.chromium.org/2151993002/diff/60001/content/browser/notific...
File content/browser/notifications/notification_message_filter.cc (right):

https://codereview.chromium.org/2151993002/diff/60001/content/browser/notific...
content/browser/notifications/notification_message_filter.cc:219:
persistent_notification_id));
Is it safe to call FindReadyRegistrationForId()? The method seems to have side
effects, notably calling
ServiceWorkerRegistration::ActivateWaitingVersionWhenReady()

I do not know what ServiceWorkerRegistration::ActivateWaitingVersionWhenReady()
does.

[Peter Beverloo, 2016-07-19 14:17:17]

lgtm

https://codereview.chromium.org/2151993002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification.h (right):

https://codereview.chromium.org/2151993002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification.h:61: // worker.
nit: maybe specify that it's used for WebAPKs?

https://codereview.chromium.org/2151993002/diff/60001/content/browser/notific...
File content/browser/notifications/notification_message_filter.cc (right):

https://codereview.chromium.org/2151993002/diff/60001/content/browser/notific...
content/browser/notifications/notification_message_filter.cc:213:
service_worker_context_);
Storing the SWCW directly (or getting it from the
PlatformNotificationContextImpl) would help here

https://codereview.chromium.org/2151993002/diff/60001/content/browser/notific...
content/browser/notifications/notification_message_filter.cc:219:
persistent_notification_id));
On 2016/07/15 19:48:45, pkotwicz wrote:
> Is it safe to call FindReadyRegistrationForId()? The method seems to have side
> effects, notably calling
> ServiceWorkerRegistration::ActivateWaitingVersionWhenReady()
> 
> I do not know what
ServiceWorkerRegistration::ActivateWaitingVersionWhenReady()
> does.

Yeah, that's fine. It's the Service Worker's registration that "owns" this call,
so it'll be running already.

https://codereview.chromium.org/2151993002/diff/60001/content/browser/notific...
content/browser/notifications/notification_message_filter.cc:233: Send(new
PlatformNotificationMsg_DidShowPersistent(request_id, false));
nit: Please add a TODO(peter) for adding UMA, as this would be another silent
failure case. A LOG(ERROR) would help too.

https://codereview.chromium.org/2151993002/diff/60001/content/browser/notific...
File content/browser/notifications/notification_message_filter.h (right):

https://codereview.chromium.org/2151993002/diff/60001/content/browser/notific...
content/browser/notifications/notification_message_filter.h:134:
ServiceWorkerContext* service_worker_context_;
nit: Use ServiceWorkerContextWrapper since we're inside of //content (no need to
use the API class).

https://codereview.chromium.org/2151993002/diff/60001/content/browser/notific...
content/browser/notifications/notification_message_filter.h:134:
ServiceWorkerContext* service_worker_context_;
nit: This should be a scoped_refptr. Another option (sorry that I didn't think
of this before) would be to add an accessor on the
PlatformNotificationContextImpl, which already has a reference. Both SGTM.

[pkotwicz, 2016-07-19 21:52:47]

Patchset #3 (id:80001) has been deleted

[pkotwicz, 2016-07-19 21:56:22]

pkotwicz@chromium.org changed reviewers:
+ sievers@chromium.org

[pkotwicz, 2016-07-19 21:56:22]

sievers@ can you please take a look at the changes in content/ that peter@ is
not an OWNER for?

[no sievers, 2016-07-19 22:44:18]

On 2016/07/19 21:56:22, pkotwicz wrote:
> sievers@ can you please take a look at the changes in content/ that peter@ is
> not an OWNER for?

lgtm

[pkotwicz, 2016-07-20 14:24:23]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-07-20 14:24:23]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2151993002/#ps100001
(title: "Merge branch 'master' into notification_scope")

[commit-bot: I haz the power, 2016-07-20 14:24:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 16:09:53]

Description was changed from

==========
This CL plumbs the service worker scope to 
PlatformNotificationServiceImpl::DisplayPersistentNotification()

The service worker scope is used to determine whether the notification
should be displayed using a WebAPK as the source or using Chrome as the
source. On Android, the notification source is displayed when a user
long-presses on a notification.

A WebAPK is used as the source of the notification if the "service
worker scope" falls within the "WebAPK scope". The "WebAPK scope"
is determined by the "scope" attribute in the WebAPK's Web Manifest
(http://html5doctor.com/web-manifest-specification/)

BUG=627842
TEST=PlatformNotificationServiceBrowserTest.PersistentNotificationServiceWorkerScope
==========

to

==========
This CL plumbs the service worker scope to 
PlatformNotificationServiceImpl::DisplayPersistentNotification()

The service worker scope is used to determine whether the notification
should be displayed using a WebAPK as the source or using Chrome as the
source. On Android, the notification source is displayed when a user
long-presses on a notification.

A WebAPK is used as the source of the notification if the "service
worker scope" falls within the "WebAPK scope". The "WebAPK scope"
is determined by the "scope" attribute in the WebAPK's Web Manifest
(http://html5doctor.com/web-manifest-specification/)

BUG=627842
TEST=PlatformNotificationServiceBrowserTest.PersistentNotificationServiceWorkerScope
==========

[commit-bot: I haz the power, 2016-07-20 16:09:54]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-07-20 16:13:04]

Description was changed from

==========
This CL plumbs the service worker scope to 
PlatformNotificationServiceImpl::DisplayPersistentNotification()

The service worker scope is used to determine whether the notification
should be displayed using a WebAPK as the source or using Chrome as the
source. On Android, the notification source is displayed when a user
long-presses on a notification.

A WebAPK is used as the source of the notification if the "service
worker scope" falls within the "WebAPK scope". The "WebAPK scope"
is determined by the "scope" attribute in the WebAPK's Web Manifest
(http://html5doctor.com/web-manifest-specification/)

BUG=627842
TEST=PlatformNotificationServiceBrowserTest.PersistentNotificationServiceWorkerScope
==========

to

==========
This CL plumbs the service worker scope to
PlatformNotificationServiceImpl::DisplayPersistentNotification()

The service worker scope is used to determine whether the notification
should be displayed using a WebAPK as the source or using Chrome as the
source. On Android, the notification source is displayed when a user
long-presses on a notification.

A WebAPK is used as the source of the notification if the "service
worker scope" falls within the "WebAPK scope". The "WebAPK scope"
is determined by the "scope" attribute in the WebAPK's Web Manifest
(http://html5doctor.com/web-manifest-specification/)

BUG=627842
TEST=PlatformNotificationServiceBrowserTest.PersistentNotificationServiceWorkerScope

Committed: https://crrev.com/1f71208bc7377d187085071782ca34c6de8e2c1e
Cr-Commit-Position: refs/heads/master@{#406570}
==========

[commit-bot: I haz the power, 2016-07-20 16:13:05]

Patchset 3 (id:??) landed as
https://crrev.com/1f71208bc7377d187085071782ca34c6de8e2c1e
Cr-Commit-Position: refs/heads/master@{#406570}