Start pulling away notification logic from ChromeContentBrowserClient.

Start pulling away notification logic from ChromeContentBrowserClient.

This moves a bunch of notification permission logic (specifically for
extensions) from ChromeContentBrowserClient to a new object, the
PlatformNotificationService, which is a singleton in the browser process.

BUG=439950
Committed: https://crrev.com/c26c6c6774f6c150663c33ee107938c5470568a0
Cr-Commit-Position: refs/heads/master@{#307694}

[Peter Beverloo, 2014-12-02 16:39:24]

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

[Peter Beverloo, 2014-12-02 16:39:24]

I'm a bit in flux about this patch.

I quite like pulling our logic away from ContentBrowserClient, as it shouldn't
be there. It also allows us to move Profile-agnostic parts of Notifications away
from the DesktopNotificationService.

What I'm less fond of is the duplication in name (Platform v.s.
DesktopNotificationService). I imagine that we can rename the latter of them
BrowserContextNotificationService, in line with the plan to move stuff to
components/.

Furthermore, I'd also still like to pull away the extensions stuff in a separate
class. That way we can have:

  (1) PlatformNotificationServiceImpl, singleton class feeding
ContentBrowserClient.
  (2) BrowserContextNotificationService, per-profile (BC) instance, superseding
the existing DesktopNotificationService.
  (3) ExtensionNotificationService, not sure what should own this. Is there an
extensions master-object of some kind?

[dewittj, 2014-12-02 17:22:59]

> What I'm less fond of is the duplication in name
Notification is also a heavily overloaded term in Chrome.  Wonder if
UserNotificationService is better?

Also, seems like the new PlatformNotificationService is duplicating the job of
NotificationUIManager; do you plan to merge them over time?

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

https://codereview.chromium.org/774573003/diff/20001/content/browser/notifica...
content/browser/notifications/notification_message_filter.cc:64: } else
nit: braces around else clause

https://codereview.chromium.org/774573003/diff/20001/content/public/browser/p...
File content/public/browser/platform_notification_service.h (right):

https://codereview.chromium.org/774573003/diff/20001/content/public/browser/p...
content/public/browser/platform_notification_service.h:43: base::Closure*
cancel_callback) = 0;
It's unclear that this is an outparam, can you document that, or even better
make it the return value?

[Peter Beverloo, 2014-12-05 20:03:43]

No, not really. My view is:

    (1) PlatformNotificationService(Impl) -- Interface made available through
the ContentBrowserClient. Solely responsible for Web Notifications, optionally
through extensions. It's a singleton.

    (2) MessageCenterProfileNotificationService -- Essentially today's
DesktopNotificationService, minus the part that services the
ContentBrowserClient. It's owned by the profile and deals with the preferences.

    (3) NotificationUIManager -- Interface responsible for the actual displaying
of notifications. Directly used by non-Web Notification consumers. Two
implementations, MessageCenter and Android.

This means that (1) and (3) can move to a component (they don't need to know
anything about the Profile). I'd like to clean up the code in the current
directory first to ensure that we can have a clean move.

My immediate motivation for (1) is that I'd like to add two more methods:
DisplayPersistentNotification and ClosePersistentNotification. This'd be tedious
to do on ContentBrowserClient, where we already have a too-large presence. These
are notifications which don't have a delegate, and will be dispatched through
NotificationEventDispatcher.

https://codereview.chromium.org/728763003/

Perhaps it's useful to have a chat about this? :-)

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

https://codereview.chromium.org/774573003/diff/20001/content/browser/notifica...
content/browser/notifications/notification_message_filter.cc:64: } else
On 2014/12/02 17:22:59, dewittj wrote:
> nit: braces around else clause

Done.

https://codereview.chromium.org/774573003/diff/20001/content/public/browser/p...
File content/public/browser/platform_notification_service.h (right):

https://codereview.chromium.org/774573003/diff/20001/content/public/browser/p...
content/public/browser/platform_notification_service.h:43: base::Closure*
cancel_callback) = 0;
On 2014/12/02 17:22:59, dewittj wrote:
> It's unclear that this is an outparam, can you document that, or even better
> make it the return value?

I've documented it in the comment. I don't think that returning the callback is
a great solution, mostly because the name implies that, if it were to return
anything, it'd be a boolean. A naive check like:

if (service_->DisplayNotification(...)) { }

would succeed just fine when returning the closure...

We could name it something verbose like
DisplayNotificationAndReturnCloseClosure, though.

[Peter Beverloo, 2014-12-08 14:26:58]

On 2014/12/05 20:03:43, Peter Beverloo wrote:
> No, not really. My view is:
> 
>     (1) PlatformNotificationService(Impl) -- Interface made available through
> the ContentBrowserClient. Solely responsible for Web Notifications, optionally
> through extensions. It's a singleton.
> 
>     (2) MessageCenterProfileNotificationService -- Essentially today's
> DesktopNotificationService, minus the part that services the
> ContentBrowserClient. It's owned by the profile and deals with the
preferences.
> 
>     (3) NotificationUIManager -- Interface responsible for the actual
displaying
> of notifications. Directly used by non-Web Notification consumers. Two
> implementations, MessageCenter and Android.
> 
> This means that (1) and (3) can move to a component (they don't need to know
> anything about the Profile). I'd like to clean up the code in the current
> directory first to ensure that we can have a clean move.
> 
> My immediate motivation for (1) is that I'd like to add two more methods:
> DisplayPersistentNotification and ClosePersistentNotification. This'd be
tedious
> to do on ContentBrowserClient, where we already have a too-large presence.
These
> are notifications which don't have a delegate, and will be dispatched through
> NotificationEventDispatcher.
> 
> https://codereview.chromium.org/728763003/
> 
> Perhaps it's useful to have a chat about this? :-)
> 
>
https://codereview.chromium.org/774573003/diff/20001/content/browser/notifica...
> File content/browser/notifications/notification_message_filter.cc (right):
> 
>
https://codereview.chromium.org/774573003/diff/20001/content/browser/notifica...
> content/browser/notifications/notification_message_filter.cc:64: } else
> On 2014/12/02 17:22:59, dewittj wrote:
> > nit: braces around else clause
> 
> Done.
> 
>
https://codereview.chromium.org/774573003/diff/20001/content/public/browser/p...
> File content/public/browser/platform_notification_service.h (right):
> 
>
https://codereview.chromium.org/774573003/diff/20001/content/public/browser/p...
> content/public/browser/platform_notification_service.h:43: base::Closure*
> cancel_callback) = 0;
> On 2014/12/02 17:22:59, dewittj wrote:
> > It's unclear that this is an outparam, can you document that, or even better
> > make it the return value?
> 
> I've documented it in the comment. I don't think that returning the callback
is
> a great solution, mostly because the name implies that, if it were to return
> anything, it'd be a boolean. A naive check like:
> 
> if (service_->DisplayNotification(...)) { }
> 
> would succeed just fine when returning the closure...
> 
> We could name it something verbose like
> DisplayNotificationAndReturnCloseClosure, though.

Further background in the following document:
 
https://docs.google.com/document/d/1tjLoJes1PwmZymaIHJ67nqGSQ3qs1TKGsQ2XbMv1E...

[Peter Beverloo, 2014-12-09 19:41:05]

ping?

[dewittj, 2014-12-09 19:47:54]

lgtm

[Peter Beverloo, 2014-12-09 21:54:07]

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

[Peter Beverloo, 2014-12-09 21:54:08]

Thanks! +avi for content/

[Avi (use Gerrit), 2014-12-10 00:18:39]

lgtm

stampity stamp

https://codereview.chromium.org/774573003/diff/80001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/774573003/diff/80001/content/public/browser/c...
content/public/browser/content_browser_client.h:419: // Notifications to the
user. The embedder can return NULL if they don't
nullptr?

[Peter Beverloo, 2014-12-10 12:14:13]

https://codereview.chromium.org/774573003/diff/80001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/774573003/diff/80001/content/public/browser/c...
content/public/browser/content_browser_client.h:419: // Notifications to the
user. The embedder can return NULL if they don't
On 2014/12/10 00:18:39, Avi wrote:
> nullptr?

Done.

[Peter Beverloo, 2014-12-10 12:14:19]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-12-10 12:14:59]

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

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

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-10 14:14:42]

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