Introduce the Blink NotificationService, move permission checks there

Introduce the Blink NotificationService, move permission checks there

This introduces the Mojo NotificationService in Blink and moves
permission checks over from the IPC code-path to the Mojo code-path.

BUG=595685
Committed: https://crrev.com/c5987942de604fb5d51ef70e208931b713522955
Cr-Commit-Position: refs/heads/master@{#394185}

[Peter Beverloo, 2016-05-05 16:43:52]

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

[Peter Beverloo, 2016-05-05 16:43:53]

+sammc for a question

This will be the first step in moving Notifications over, full picture view can
be seen here:

https://codereview.chromium.org/1904163002/

https://codereview.chromium.org/1948223004/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/NotificationManager.cpp
(right):

https://codereview.chromium.org/1948223004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationManager.cpp:47:
Platform::current()->serviceRegistry()->connectToRemoteService(mojo::GetProxy(&m_service));
@sammc -

There will be one NotificationManager per ExecutionContext, and since
Notifications are available in all kinds of Web Workers this does *not* have to
be the main thread, which is different from other Mojo consumers in Blink today.

In effect, a crash happens because connectToRemoteService() is used on the wrong
thread when using the feature from a worker.

Could you share your ideas about the best way of dealing with this? (I.e. thread
jump here/when connecting, different registry, another solution?)

[Peter Beverloo, 2016-05-05 16:47:34]

+Ken as he might know the answer too

[Ken Rockot(use gerrit already), 2016-05-05 16:57:42]

On 2016/05/05 at 16:47:34, peter wrote:
> +Ken as he might know the answer too

The right thing to do would be to thread-hop for the call to
connectToRemoteService. This should be pretty painless because the
InterfaceRequest returned by GetProxy is safe to pass to any thread.

[dcheng, 2016-05-05 16:59:28]

Please update the OWNERS file to add mojom security reviewers. Thanks!

[Peter Beverloo, 2016-05-05 17:49:04]

peter@chromium.org changed reviewers:
+ dcheng@chromium.org, esprehn@chromium.org, mvanouwerkerk@chromium.org
- sammc@chromium.org

[Peter Beverloo, 2016-05-05 17:49:04]

+mvanouwerkerk for notifications
+esprehn for blink_service_registry_impl.cc
+dcheng for security

Elliot, a Mojo service must be connected to on the main thread, so a jump to
connectToRemoteService() has to happen somewhere. Per Ken's comment, this is
safe to do since it's just a request.

Notifications may live on any ExecutionContext thread because they are available
in workers as well as documents.

Three ideas on how to do this would be:
  (1) Explicitly jump in Blink code where this is necessary. This means we'd end
up with calls to the main thread in a number of different places.
  (2) Separate connectToRemoteService() and
connectToRemoteServiceOnMainThread(). This means NotificationManager.cpp needs
an if-statement depending on whether or not it's on the main thread, possibly in
other places too.
  (3) Implicitly jump as part of connectToRemoteService() if it was called on a
non-main thread.

I've opted for (3) in this CL. If this works for you, I can propagate comments
in headers files?

[haraken, 2016-05-06 00:41:12]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-05-06 00:41:12]

https://codereview.chromium.org/1948223004/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/notifications/OWNERS (right):

https://codereview.chromium.org/1948223004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/notifications/OWNERS:4: # introducing
new sandbox escapes.

dcheng@: Do we need to add this to all modules/X/OWNERS?

[dcheng, 2016-05-06 01:06:36]

https://codereview.chromium.org/1948223004/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/notifications/OWNERS (right):

https://codereview.chromium.org/1948223004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/notifications/OWNERS:4: # introducing
new sandbox escapes.
On 2016/05/06 at 00:41:12, haraken wrote:
> dcheng@: Do we need to add this to all modules/X/OWNERS?

Yeah. I talked with peter@ and dpranke@ today about the changes I'm going to
make to owners.py, and I think I have agreement on what to do. So this shouldn't
be necessary in a few days.

[Sam McNally, 2016-05-06 05:14:09]

https://codereview.chromium.org/1948223004/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/NotificationManager.cpp
(right):

https://codereview.chromium.org/1948223004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationManager.cpp:47:
Platform::current()->serviceRegistry()->connectToRemoteService(mojo::GetProxy(&m_service));
On 2016/05/05 16:43:53, Peter Beverloo wrote:
> @sammc -
> 
> There will be one NotificationManager per ExecutionContext, and since
> Notifications are available in all kinds of Web Workers this does *not* have
to
> be the main thread, which is different from other Mojo consumers in Blink
today.
> 
> In effect, a crash happens because connectToRemoteService() is used on the
wrong
> thread when using the feature from a worker.
> 
> Could you share your ideas about the best way of dealing with this? (I.e.
thread
> jump here/when connecting, different registry, another solution?)

Platform::serviceRegistry() should be the right place to connect to process-wide
services from any thread. Adding plumbing to the main thread SGTM.

However, if the service has some scoping to the particular context, it should
use a ServiceRegistry specific to that context. For frames, this is
LocalFrame::serviceRegistry(); for service workers, there's a ServiceRegistry in
content::ServiceWorkerContextClient, but it isn't exposed to Blink; for other
workers there isn't one yet. The existing worker-exposed services workaround
this by using the per-process ServiceRegistry (with plumbing to the main thread
in content/renderer) and including the context in the messages (e.g.
PermissionService).

In the longer term it may be useful to add a ServiceRegistry accessor to
something more agnostic like ExecutionContext to reduce the amount of
per-context-type boilerplate.

https://codereview.chromium.org/1948223004/diff/20001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/notifications/notification_service.mojom
(right):

https://codereview.chromium.org/1948223004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/notifications/notification_service.mojom:14:
[Sync] GetPermissionStatus(string origin) => (PermissionStatus status);
This seems like it belongs in PermissionService.

[Michael van Ouwerkerk, 2016-05-06 10:46:07]

https://codereview.chromium.org/1948223004/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/NotificationManager.cpp
(right):

https://codereview.chromium.org/1948223004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationManager.cpp:47:
Platform::current()->serviceRegistry()->connectToRemoteService(mojo::GetProxy(&m_service));
On 2016/05/06 05:14:09, Sam McNally wrote:
> On 2016/05/05 16:43:53, Peter Beverloo wrote:
> > @sammc -
> > 
> > There will be one NotificationManager per ExecutionContext, and since
> > Notifications are available in all kinds of Web Workers this does *not* have
> to
> > be the main thread, which is different from other Mojo consumers in Blink
> today.
> > 
> > In effect, a crash happens because connectToRemoteService() is used on the
> wrong
> > thread when using the feature from a worker.
> > 
> > Could you share your ideas about the best way of dealing with this? (I.e.
> thread
> > jump here/when connecting, different registry, another solution?)
> 
> Platform::serviceRegistry() should be the right place to connect to
process-wide
> services from any thread. Adding plumbing to the main thread SGTM.
> 
> However, if the service has some scoping to the particular context, it should
> use a ServiceRegistry specific to that context. For frames, this is
> LocalFrame::serviceRegistry(); for service workers, there's a ServiceRegistry
in
> content::ServiceWorkerContextClient, but it isn't exposed to Blink; for other
> workers there isn't one yet. The existing worker-exposed services workaround
> this by using the per-process ServiceRegistry (with plumbing to the main
thread
> in content/renderer) and including the context in the messages (e.g.
> PermissionService).
> 
> In the longer term it may be useful to add a ServiceRegistry accessor to
> something more agnostic like ExecutionContext to reduce the amount of
> per-context-type boilerplate.

This would be really useful. Where would the browser-side counterpart of an
ExecutionContext-owned endpoint live, in C++ and in Java?

[Michael van Ouwerkerk, 2016-05-06 11:45:20]

notifications lgtm with nit

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

https://codereview.chromium.org/1948223004/diff/20001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:113:
base::Bind(&PlatformNotificationContextImpl::CreateServiceOnIO, this,
Should this use a weak ptr?

https://codereview.chromium.org/1948223004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/NotificationManager.cpp
(right):

https://codereview.chromium.org/1948223004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationManager.cpp:15:
#define STATIC_ASSERT_ENUM(a, b) \
That is beautiful. Can we keep it?

https://codereview.chromium.org/1948223004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/NotificationManager.h
(right):

https://codereview.chromium.org/1948223004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationManager.h:46:
NotificationManager(ExecutionContext*);
explicit

[Peter Beverloo, 2016-05-06 14:51:33]

Thank you for the comments! PTAL

> However, if the service has some scoping to the particular context, it
> should use a ServiceRegistry specific to that context. For frames, this
> is LocalFrame::serviceRegistry(); for service workers, there's a
> ServiceRegistry in content::ServiceWorkerContextClient, but it isn't
> exposed to Blink; for other workers there isn't one yet.

Agreed. In case of notifications, which are available in all contexts, we don't
need to scope to any particular context except in two situations:

  (1) Requesting the user's permission, which already takes a path orthogonal to
this.
  (2) Dispatching the event on the Service Worker, for which we will, for now,
use a separate service.

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

https://codereview.chromium.org/1948223004/diff/20001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:113:
base::Bind(&PlatformNotificationContextImpl::CreateServiceOnIO, this,
On 2016/05/06 11:45:20, Michael van Ouwerkerk wrote:
> Should this use a weak ptr?

There's no need to keep the object alive for this, so yeah, we can. Done.

https://codereview.chromium.org/1948223004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/NotificationManager.cpp
(right):

https://codereview.chromium.org/1948223004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationManager.cpp:15:
#define STATIC_ASSERT_ENUM(a, b) \
On 2016/05/06 11:45:20, Michael van Ouwerkerk wrote:
> That is beautiful. Can we keep it?

Done.

https://codereview.chromium.org/1948223004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/NotificationManager.h
(right):

https://codereview.chromium.org/1948223004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationManager.h:46:
NotificationManager(ExecutionContext*);
On 2016/05/06 11:45:20, Michael van Ouwerkerk wrote:
> explicit

Done.

https://codereview.chromium.org/1948223004/diff/20001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/notifications/notification_service.mojom
(right):

https://codereview.chromium.org/1948223004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/notifications/notification_service.mojom:14:
[Sync] GetPermissionStatus(string origin) => (PermissionStatus status);
On 2016/05/06 05:14:09, Sam McNally wrote:
> This seems like it belongs in PermissionService.

Conceptually it does, but the notification permission checks go beyond what the
permission service (and back-ends) currently offer due to tying in to
extensions. This is a refactoring we will do later.

[mlamouri (slow - plz ping), 2016-05-07 12:55:24]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-05-07 12:55:25]

https://codereview.chromium.org/1948223004/diff/20001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/notifications/notification_service.mojom
(right):

https://codereview.chromium.org/1948223004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/notifications/notification_service.mojom:14:
[Sync] GetPermissionStatus(string origin) => (PermissionStatus status);
On 2016/05/06 at 14:51:33, Peter Beverloo wrote:
> On 2016/05/06 05:14:09, Sam McNally wrote:
> > This seems like it belongs in PermissionService.
> 
> Conceptually it does, but the notification permission checks go beyond what
the permission service (and back-ends) currently offer due to tying in to
extensions. This is a refactoring we will do later.

I don't think extensions is a problem to use the permission service. The
permission service will ultimately call the chrome's PermissionManager which
will then call the notification permission context which will deal with the
extension stuff. Last I checked, geolocation hooks into extensions too.

This said, I would prefer to avoid adding this sync method to the permission
service because it's only used by notification code and I wouldn't want to
introduce this to the permission service with the risk of having new uses of it
and mostly, adding constraints to permission code coming from old notification
api debts.

[mlamouri (slow - plz ping), 2016-05-09 09:26:51]

mlamouri@chromium.org changed reviewers:
- mlamouri@chromium.org

[dcheng, 2016-05-10 06:54:46]

https://codereview.chromium.org/1948223004/diff/40001/content/browser/notific...
File content/browser/notifications/blink_notification_service_impl.cc (right):

https://codereview.chromium.org/1948223004/diff/40001/content/browser/notific...
content/browser/notifications/blink_notification_service_impl.cc:21:
PlatformNotificationService* service() {
Nit: Should be called Service()

https://codereview.chromium.org/1948223004/diff/40001/content/browser/notific...
content/browser/notifications/blink_notification_service_impl.cc:50: const
mojo::String& origin,
Out of curiosity, is it possible to use url::Origin here instead? Why or why
not? =)

https://codereview.chromium.org/1948223004/diff/40001/content/browser/notific...
File content/browser/notifications/platform_notification_context_impl.h (right):

https://codereview.chromium.org/1948223004/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.h:178:
std::unordered_set<BlinkNotificationServiceImpl*> services_;
Can we store std::unique_ptr's in the set?

https://codereview.chromium.org/1948223004/diff/40001/content/renderer/mojo/b...
File content/renderer/mojo/blink_service_registry_impl.cc (right):

https://codereview.chromium.org/1948223004/diff/40001/content/renderer/mojo/b...
content/renderer/mojo/blink_service_registry_impl.cc:30:
base::Passed(std::move(handle))));
base::Passed(&handle) should work

https://codereview.chromium.org/1948223004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/NotificationManager.cpp
(right):

https://codereview.chromium.org/1948223004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationManager.cpp:11:
#include "public/platform/modules/permissions/permission_status.mojom.h"
Whoa. How come we need both? We plan on making it a PRESUBMIT error to use the
non-Blink variant of bindings in //third_party/WebKit.

[Peter Beverloo, 2016-05-10 13:32:55]

https://codereview.chromium.org/1948223004/diff/40001/content/browser/notific...
File content/browser/notifications/blink_notification_service_impl.cc (right):

https://codereview.chromium.org/1948223004/diff/40001/content/browser/notific...
content/browser/notifications/blink_notification_service_impl.cc:21:
PlatformNotificationService* service() {
On 2016/05/10 06:54:46, dcheng wrote:
> Nit: Should be called Service()

Done.

https://codereview.chromium.org/1948223004/diff/40001/content/browser/notific...
content/browser/notifications/blink_notification_service_impl.cc:50: const
mojo::String& origin,
On 2016/05/10 06:54:46, dcheng wrote:
> Out of curiosity, is it possible to use url::Origin here instead? Why or why
> not? =)

We could assuming Blink can depend on //url, with the complexity of GURL vs.
KURL. (So we would have to do the KURL -> String -> GURL jump anyway.) I don't
mind looking in to it, but it seems out of scope for this CL.

https://codereview.chromium.org/1948223004/diff/40001/content/browser/notific...
File content/browser/notifications/platform_notification_context_impl.h (right):

https://codereview.chromium.org/1948223004/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.h:178:
std::unordered_set<BlinkNotificationServiceImpl*> services_;
On 2016/05/10 06:54:46, dcheng wrote:
> Can we store std::unique_ptr's in the set?

Yes, but we lose the properties of a set. Since the number of services will
generally be very small, that should be fine. Done.

https://codereview.chromium.org/1948223004/diff/40001/content/renderer/mojo/b...
File content/renderer/mojo/blink_service_registry_impl.cc (right):

https://codereview.chromium.org/1948223004/diff/40001/content/renderer/mojo/b...
content/renderer/mojo/blink_service_registry_impl.cc:30:
base::Passed(std::move(handle))));
On 2016/05/10 06:54:46, dcheng wrote:
> base::Passed(&handle) should work

Done.

https://codereview.chromium.org/1948223004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/NotificationManager.cpp
(right):

https://codereview.chromium.org/1948223004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationManager.cpp:11:
#include "public/platform/modules/permissions/permission_status.mojom.h"
On 2016/05/10 06:54:46, dcheng wrote:
> Whoa. How come we need both? We plan on making it a PRESUBMIT error to use the
> non-Blink variant of bindings in //third_party/WebKit.

This is a transitional step only, because the Permission API implementation in
Blink still uses the non-Blink variants, and the Notification class already
contains some code for that type (i.e. permissionString).

Once they update, this can be removed and we can use the Blink variant directly.

[dcheng, 2016-05-11 21:26:39]

dcheng@chromium.org changed reviewers:
+ cmumford@chromium.org

[dcheng, 2016-05-11 21:26:40]

+cmumford as well, who I'm told is working on typemapping for url.mojom.Origin
in Blink.

https://codereview.chromium.org/1948223004/diff/40001/content/browser/notific...
File content/browser/notifications/blink_notification_service_impl.cc (right):

https://codereview.chromium.org/1948223004/diff/40001/content/browser/notific...
content/browser/notifications/blink_notification_service_impl.cc:50: const
mojo::String& origin,
On 2016/05/10 at 13:32:55, Peter Beverloo wrote:
> On 2016/05/10 06:54:46, dcheng wrote:
> > Out of curiosity, is it possible to use url::Origin here instead? Why or why
> > not? =)
> 
> We could assuming Blink can depend on //url, with the complexity of GURL vs.
KURL. (So we would have to do the KURL -> String -> GURL jump anyway.) I don't
mind looking in to it, but it seems out of scope for this CL.

I guess this is actually a SecurityOrigin we're trying to transfer. How
'compatible' is url::Origin with SecurityOrigin?

https://codereview.chromium.org/1948223004/diff/40001/content/browser/notific...
File content/browser/notifications/platform_notification_context_impl.h (right):

https://codereview.chromium.org/1948223004/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.h:178:
std::unordered_set<BlinkNotificationServiceImpl*> services_;
On 2016/05/10 at 13:32:55, Peter Beverloo wrote:
> On 2016/05/10 06:54:46, dcheng wrote:
> > Can we store std::unique_ptr's in the set?
> 
> Yes, but we lose the properties of a set. Since the number of services will
generally be very small, that should be fine. Done.

It seems strictly better than having to manually delete pointers though =)

If the algorithmic complexity is important, one workaround is to do an
unordered_map<BlinkNotificationServiceImpl*,
std::unique_ptr<BlinkNotificationServiceImpl>>. A bit... meh, but it works. And
if we don't care... maybe vector is just as good? =)

[Peter Beverloo, 2016-05-12 19:58:04]

+jam for content DEPS and render_process_host_impl.cc

Friendly ping to the other reviewers, is anything blocking you from doing a
review?

I would like to move forward with the refactoring, which will definitely involve
plenty of more work, but this isn't really getting anywhere now :-).

[Peter Beverloo, 2016-05-12 19:58:23]

Description was changed from

==========
Introduce the Blink NotificationService, move permission checks there

This introduces the Mojo NotificationService in Blink and moves
permission checks over from the IPC code-path to the Mojo code-path.

BUG=595685
==========

to

==========
Introduce the Blink NotificationService, move permission checks there

This introduces the Mojo NotificationService in Blink and moves
permission checks over from the IPC code-path to the Mojo code-path.

BUG=595685
==========

[Peter Beverloo, 2016-05-12 19:58:23]

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

[esprehn, 2016-05-12 21:03:47]

https://codereview.chromium.org/1948223004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1948223004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.cpp:56: #include
"public/platform/modules/permissions/permission_status.mojom.h"
.mojom-blink.h, you want to use the blink style bindings.

https://codereview.chromium.org/1948223004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/NotificationManager.cpp
(right):

https://codereview.chromium.org/1948223004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationManager.cpp:11:
#include "public/platform/modules/permissions/permission_status.mojom.h"
remove this, only the blink style bindings are allowed inside blink.

https://codereview.chromium.org/1948223004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationManager.cpp:21:
STATIC_ASSERT_ENUM(mojom::PermissionStatus::LAST,
mojom::blink::PermissionStatus::LAST);
remove this, the code generator enforces this for you. Also blink should only
use the blink bindings.

https://codereview.chromium.org/1948223004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationManager.cpp:73:
String NotificationManager::getOriginString() const
this only has one caller, can we just write the incantation there? We can add
the method when we do it more than once

[jam, 2016-05-13 01:00:20]

https://codereview.chromium.org/1948223004/diff/80001/content/browser/notific...
File content/browser/notifications/blink_notification_service_impl.cc (right):

https://codereview.chromium.org/1948223004/diff/80001/content/browser/notific...
content/browser/notifications/blink_notification_service_impl.cc:27:
BlinkNotificationServiceImpl::BlinkNotificationServiceImpl(
this class seems to serve little purpose, i.e. just dispatching mojo calls in
content to call embedder. it seems it's simpler to implement this on chrome's
PlatformNotificationServiceImpl directly. that would be similar to how we have
some webkit delegate APIs like webautofillclient that are implemented by the
embedder and not content

[Peter Beverloo, 2016-05-13 01:45:42]

On 2016/05/13 01:00:20, jam wrote:
>
https://codereview.chromium.org/1948223004/diff/80001/content/browser/notific...
> File content/browser/notifications/blink_notification_service_impl.cc (right):
> 
>
https://codereview.chromium.org/1948223004/diff/80001/content/browser/notific...
> content/browser/notifications/blink_notification_service_impl.cc:27:
> BlinkNotificationServiceImpl::BlinkNotificationServiceImpl(
> this class seems to serve little purpose, i.e. just dispatching mojo calls in
> content to call embedder. it seems it's simpler to implement this on chrome's
> PlatformNotificationServiceImpl directly. that would be similar to how we have
> some webkit delegate APIs like webautofillclient that are implemented by the
> embedder and not content

This is just the first CL, plenty more functionality (much of which is
implemented in //content) is to follow. Full context here:

https://codereview.chromium.org/1904163002/

In short, functionality provided by the NotificationMessageFilter today will
move to the new class in iteration.

[Peter Beverloo, 2016-05-13 15:10:13]

PTAL.

https://codereview.chromium.org/1948223004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1948223004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.cpp:56: #include
"public/platform/modules/permissions/permission_status.mojom.h"
On 2016/05/12 21:03:47, esprehn wrote:
> .mojom-blink.h, you want to use the blink style bindings.

Done.

https://codereview.chromium.org/1948223004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/NotificationManager.cpp
(right):

https://codereview.chromium.org/1948223004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationManager.cpp:11:
#include "public/platform/modules/permissions/permission_status.mojom.h"
On 2016/05/12 21:03:47, esprehn wrote:
> remove this, only the blink style bindings are allowed inside blink.

Done.

https://codereview.chromium.org/1948223004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationManager.cpp:21:
STATIC_ASSERT_ENUM(mojom::PermissionStatus::LAST,
mojom::blink::PermissionStatus::LAST);
On 2016/05/12 21:03:47, esprehn wrote:
> remove this, the code generator enforces this for you. Also blink should only
> use the blink bindings.

Done.

https://codereview.chromium.org/1948223004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationManager.cpp:73:
String NotificationManager::getOriginString() const
On 2016/05/12 21:03:47, esprehn wrote:
> this only has one caller, can we just write the incantation there? We can add
> the method when we do it more than once

Done.

[esprehn, 2016-05-13 17:22:42]

lgtm

[jam, 2016-05-13 17:55:16]

On 2016/05/13 01:45:42, Peter Beverloo wrote:
> On 2016/05/13 01:00:20, jam wrote:
> >
>
https://codereview.chromium.org/1948223004/diff/80001/content/browser/notific...
> > File content/browser/notifications/blink_notification_service_impl.cc
(right):
> > 
> >
>
https://codereview.chromium.org/1948223004/diff/80001/content/browser/notific...
> > content/browser/notifications/blink_notification_service_impl.cc:27:
> > BlinkNotificationServiceImpl::BlinkNotificationServiceImpl(
> > this class seems to serve little purpose, i.e. just dispatching mojo calls
in
> > content to call embedder. it seems it's simpler to implement this on
chrome's
> > PlatformNotificationServiceImpl directly. that would be similar to how we
have
> > some webkit delegate APIs like webautofillclient that are implemented by the
> > embedder and not content
> 
> This is just the first CL, plenty more functionality (much of which is
> implemented in //content) is to follow. Full context here:
> 
> https://codereview.chromium.org/1904163002/
> 
> In short, functionality provided by the NotificationMessageFilter today will
> move to the new class in iteration.

ah, ok. lgtm

[ncarter (slow), 2016-05-13 22:04:16]

drive-by

https://codereview.chromium.org/1948223004/diff/1/content/browser/notificatio...
File content/browser/notifications/blink_notification_service_impl.h (right):

https://codereview.chromium.org/1948223004/diff/1/content/browser/notificatio...
content/browser/notifications/blink_notification_service_impl.h:18: //
Implementation of the NotificationService used for Web Notifications. Is
jam: How confusing will it be to have both blink::mojom::NotificationService and
content/public/browser/notification_service.h? Is there anything we should do,
naming-wise, to reduce possible confusion?

[jam, 2016-05-13 22:16:47]

https://codereview.chromium.org/1948223004/diff/1/content/browser/notificatio...
File content/browser/notifications/blink_notification_service_impl.h (right):

https://codereview.chromium.org/1948223004/diff/1/content/browser/notificatio...
content/browser/notifications/blink_notification_service_impl.h:18: //
Implementation of the NotificationService used for Web Notifications. Is
On 2016/05/13 22:04:16, ncarter wrote:
> jam: How confusing will it be to have both blink::mojom::NotificationService
and
> content/public/browser/notification_service.h? Is there anything we should do,
> naming-wise, to reduce possible confusion?

it's a little confusing I agree. however content::NS is supposed to go away
(https://bugs.chromium.org/p/chromium/issues/detail?id=268984). There aren't
that many remaining, but the work has stalled.

[dcheng, 2016-05-13 22:18:18]

mojom lgtm

[haraken, 2016-05-14 00:17:52]

LGTM

[Peter Beverloo, 2016-05-17 09:54:55]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2016-05-17 09:54:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvanouwerkerk@chromium.org, esprehn@chromium.org, jam@chromium.org,
dcheng@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1948223004/#ps110001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-17 09:55:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1948223004/110001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1948223004/110001

[commit-bot: I haz the power, 2016-05-17 11:26:51]

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

[commit-bot: I haz the power, 2016-05-17 11:26:54]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Beverloo, 2016-05-17 14:43:25]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2016-05-17 14:43:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, jam@chromium.org, dcheng@chromium.org,
mvanouwerkerk@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1948223004/#ps130001
(title: " ")

[commit-bot: I haz the power, 2016-05-17 14:43:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1948223004/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1948223004/130001

[commit-bot: I haz the power, 2016-05-17 19:22:29]

Description was changed from

==========
Introduce the Blink NotificationService, move permission checks there

This introduces the Mojo NotificationService in Blink and moves
permission checks over from the IPC code-path to the Mojo code-path.

BUG=595685
==========

to

==========
Introduce the Blink NotificationService, move permission checks there

This introduces the Mojo NotificationService in Blink and moves
permission checks over from the IPC code-path to the Mojo code-path.

BUG=595685
==========

[commit-bot: I haz the power, 2016-05-17 19:22:31]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2016-05-17 19:24:00]

Description was changed from

==========
Introduce the Blink NotificationService, move permission checks there

This introduces the Mojo NotificationService in Blink and moves
permission checks over from the IPC code-path to the Mojo code-path.

BUG=595685
==========

to

==========
Introduce the Blink NotificationService, move permission checks there

This introduces the Mojo NotificationService in Blink and moves
permission checks over from the IPC code-path to the Mojo code-path.

BUG=595685

Committed: https://crrev.com/c5987942de604fb5d51ef70e208931b713522955
Cr-Commit-Position: refs/heads/master@{#394185}
==========

[commit-bot: I haz the power, 2016-05-17 19:24:02]

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

[yzshen1, 2016-05-18 15:28:55]

yzshen@chromium.org changed reviewers:
+ yzshen@chromium.org

[yzshen1, 2016-05-18 15:28:56]

https://codereview.chromium.org/1948223004/diff/130001/content/browser/notifi...
File content/browser/notifications/platform_notification_context_impl.cc
(right):

https://codereview.chromium.org/1948223004/diff/130001/content/browser/notifi...
content/browser/notifications/platform_notification_context_impl.cc:116:
weak_factory_to_io_.GetWeakPtr(), render_process_id,
Not sure whether it is relevant to the hang that you saw...but this seems wrong:
the weak pointer is dereferenced on IO thread, but the factory is destroyed on
the UI thread. That could be racy. Please see the thread safty notes in
weak_ptr.h

[Peter Beverloo, 2016-05-18 18:20:34]

A revert of this CL (patchset #8 id:130001) has been created in
https://codereview.chromium.org/1995663003/ by peter@chromium.org.

The reason for reverting is: Causing crashes for users on Windows. Reverting.

BUG=612770.