Introduce a //content/ API for dispatching notificationclick events.

Introduce a //content/ API for dispatching notificationclick events.

This patch introduces a new public API to the content layer which should be
used for dispatching notificationclick events to a specific service worker.
The logic required for selecing the appropriate service worker is included
in the implementation of this class.

We're two CLs away from being able to test this stuff, which will be done
in the form of both layout tests and browser tests.

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

[Peter Beverloo, 2014-12-05 19:23:36]

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

[Peter Beverloo, 2014-12-05 19:23:37]

+mvanouwerkerk, who hates his weekends anyway.

[Michael van Ouwerkerk, 2014-12-08 15:13:43]

https://codereview.chromium.org/728763003/diff/40001/content/browser/notifica...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/728763003/diff/40001/content/browser/notifica...
content/browser/notifications/notification_event_dispatcher_impl.cc:23: // post
a task to call |done_callback| on the UI thread.
s/|done_callback|/|callback|/

https://codereview.chromium.org/728763003/diff/40001/content/browser/notifica...
content/browser/notifications/notification_event_dispatcher_impl.cc:105:
base::Bind(callback, status));
Nit: there are multiple callbacks in this function, which one is this? Maybe
rename to dispatch_complete_callback.

https://codereview.chromium.org/728763003/diff/40001/content/browser/notifica...
content/browser/notifications/notification_event_dispatcher_impl.cc:144: void
NotificationEventDispatcherImpl::DispatchNotificationClickEvent(
Nit: this has the same name as a function in an anonymous namespace. Care to
distinguish them more clearly?

https://codereview.chromium.org/728763003/diff/40001/content/public/browser/n...
File content/public/browser/notification_event_dispatcher.h (right):

https://codereview.chromium.org/728763003/diff/40001/content/public/browser/n...
content/public/browser/notification_event_dispatcher.h:24: class CONTENT_EXPORT
NotificationEventDispatcher {
Please document somewhere that this is expected to be a singleton.

[Peter Beverloo, 2014-12-08 17:27:08]

PTAL

https://codereview.chromium.org/728763003/diff/40001/content/browser/notifica...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/728763003/diff/40001/content/browser/notifica...
content/browser/notifications/notification_event_dispatcher_impl.cc:23: // post
a task to call |done_callback| on the UI thread.
On 2014/12/08 15:13:42, Michael van Ouwerkerk wrote:
> s/|done_callback|/|callback|/

Done.

https://codereview.chromium.org/728763003/diff/40001/content/browser/notifica...
content/browser/notifications/notification_event_dispatcher_impl.cc:105:
base::Bind(callback, status));
On 2014/12/08 15:13:42, Michael van Ouwerkerk wrote:
> Nit: there are multiple callbacks in this function, which one is this? Maybe
> rename to dispatch_complete_callback.

Done.

https://codereview.chromium.org/728763003/diff/40001/content/browser/notifica...
content/browser/notifications/notification_event_dispatcher_impl.cc:144: void
NotificationEventDispatcherImpl::DispatchNotificationClickEvent(
On 2014/12/08 15:13:42, Michael van Ouwerkerk wrote:
> Nit: this has the same name as a function in an anonymous namespace. Care to
> distinguish them more clearly?

Done.

https://codereview.chromium.org/728763003/diff/40001/content/public/browser/n...
File content/public/browser/notification_event_dispatcher.h (right):

https://codereview.chromium.org/728763003/diff/40001/content/public/browser/n...
content/public/browser/notification_event_dispatcher.h:24: class CONTENT_EXPORT
NotificationEventDispatcher {
On 2014/12/08 15:13:43, Michael van Ouwerkerk wrote:
> Please document somewhere that this is expected to be a singleton.

Done.

[Michael van Ouwerkerk, 2014-12-08 18:55:33]

lgtm

[Peter Beverloo, 2014-12-08 18:57:44]

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

[Peter Beverloo, 2014-12-08 18:57:44]

Cool, thanks!

+michaeln for Service Workers changes.

+avi for content/ review. This is about the static methods on BrowserContext
discussion we had the other day (..and does not add one! :-)).

[Avi (use Gerrit), 2014-12-08 19:28:18]

lgtm

owner stamp

[michaeln, 2014-12-09 02:18:38]

sw lgtm

[Peter Beverloo, 2014-12-09 11:34:53]

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

[Peter Beverloo, 2014-12-09 11:34:54]

+mkwst for IPC

[Mike West, 2014-12-09 11:47:30]

https://codereview.chromium.org/728763003/diff/60001/content/common/service_w...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/728763003/diff/60001/content/common/service_w...
content/common/service_worker/service_worker_messages.h:382:
content::ShowDesktopNotificationHostMsgParams)
It's not clear what aspects of this struct you're actually using here. Do you
really need all of it?

[Peter Beverloo, 2014-12-09 11:51:59]

https://codereview.chromium.org/728763003/diff/60001/content/common/service_w...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/728763003/diff/60001/content/common/service_w...
content/common/service_worker/service_worker_messages.h:382:
content::ShowDesktopNotificationHostMsgParams)
On 2014/12/09 11:47:30, Mike West wrote:
> It's not clear what aspects of this struct you're actually using here. Do you
> really need all of it?

It's being used in service_worker_script_context.cc. I also added a TODO in the
file for ShowDesktopNotificationHostMsgParams to rename it to
PlatformNotificationData, and probably pull out the Bitmap (Icon) as part of
that.

Happy to send you a CL for that later today?

[Mike West, 2014-12-09 12:00:56]

On 2014/12/09 11:51:59, Peter Beverloo wrote:
>
https://codereview.chromium.org/728763003/diff/60001/content/common/service_w...
> File content/common/service_worker/service_worker_messages.h (right):
> 
>
https://codereview.chromium.org/728763003/diff/60001/content/common/service_w...
> content/common/service_worker/service_worker_messages.h:382:
> content::ShowDesktopNotificationHostMsgParams)
> On 2014/12/09 11:47:30, Mike West wrote:
> > It's not clear what aspects of this struct you're actually using here. Do
you
> > really need all of it?
> 
> It's being used in service_worker_script_context.cc. I also added a TODO in
the
> file for ShowDesktopNotificationHostMsgParams to rename it to
> PlatformNotificationData, and probably pull out the Bitmap (Icon) as part of
> that.
> 
> Happy to send you a CL for that later today?

Sounds good. LGTM.

[Peter Beverloo, 2014-12-09 12:12:44]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-12-09 12:13:34]

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

[commit-bot: I haz the power, 2014-12-09 12:53:15]

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

[commit-bot: I haz the power, 2014-12-09 12:53:16]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Peter Beverloo, 2014-12-09 13:16:24]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-12-09 13:16:45]

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

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

Committed patchset #5 (id:80001)

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

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