Mojoify the notificationclick and notificationclose events

Mojoify the notificationclick and notificationclose events

BUG=595685
Review-Url: https://codereview.chromium.org/2682353002
Cr-Commit-Position: refs/heads/master@{#450007}
Committed: https://chromium.googlesource.com/chromium/src/+/f8ea4c45d43e8e4871f9a51f74fa8d74618c0886

[Peter Beverloo, 2017-02-09 17:29:31]

The CQ bit was checked by peter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 17:29:58]

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

[Peter Beverloo, 2017-02-09 17:32:44]

peter@chromium.org changed reviewers:
+ awdf@chromium.org, shimazu@chromium.org

[Peter Beverloo, 2017-02-09 17:32:44]

+awdf for notifications
+shimazu for service workers - thank you for the help during BlinkOn!

[commit-bot: I haz the power, 2017-02-09 19:21:32]

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

[commit-bot: I haz the power, 2017-02-09 19:21:33]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2017-02-10 03:21:28]

https://codereview.chromium.org/2682353002/diff/1/content/browser/notificatio...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/2682353002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_event_dispatcher_impl.cc:220:
service_worker, request_id));
Err, this and line 288 need to use base::Unretained(service_worker.get()).
Consider it changed :>. Will send a patch for PushEvent too.

[shimazu, 2017-02-10 08:09:30]

https://codereview.chromium.org/2682353002/diff/1/content/renderer/service_wo...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2682353002/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.cc:739: :
SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED,
You can use "EventResultToStatus(result)" here.

https://codereview.chromium.org/2682353002/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.cc:755: :
SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED,
ditto

https://codereview.chromium.org/2682353002/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.cc:1072:
ToWebNotificationData(notification_data), action_index, web_reply);
I think you can use blink.mojom.Notification directly instead of
ToWebNotificationData. Using PlatformNotificationData is really straightforward,
but I'd like to have less [Native]s and like to remove _messages.h.
ConsoleMessage might be a good reference for type-mapping to a blink type.
https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/console_me...

Creating another patch to introduce the typemap is also possible.
WDYT?

[Peter Beverloo, 2017-02-10 16:41:06]

Thanks!

https://codereview.chromium.org/2682353002/diff/1/content/browser/notificatio...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/2682353002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_event_dispatcher_impl.cc:220:
service_worker, request_id));
On 2017/02/10 03:21:28, Peter Beverloo wrote:
> Err, this and line 288 need to use base::Unretained(service_worker.get()).
> Consider it changed :>. Will send a patch for PushEvent too.

Done.

https://codereview.chromium.org/2682353002/diff/1/content/renderer/service_wo...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2682353002/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.cc:739: :
SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED,
On 2017/02/10 08:09:29, shimazu wrote:
> You can use "EventResultToStatus(result)" here.

Done.

https://codereview.chromium.org/2682353002/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.cc:755: :
SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED,
On 2017/02/10 08:09:29, shimazu wrote:
> ditto

Done.

https://codereview.chromium.org/2682353002/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.cc:1072:
ToWebNotificationData(notification_data), action_index, web_reply);
On 2017/02/10 08:09:29, shimazu wrote:
> I think you can use blink.mojom.Notification directly instead of
> ToWebNotificationData. Using PlatformNotificationData is really
straightforward,
> but I'd like to have less [Native]s and like to remove _messages.h.
> ConsoleMessage might be a good reference for type-mapping to a blink type.
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/console_me...
> 
> Creating another patch to introduce the typemap is also possible.
> WDYT?

This relates to the layering issue that we spoke about last week.

Keeping WebNotificationData as the type used in Blink is an interesting idea.
The type map configuration would be a bit tricky due to the mojo variants;

  - We need WebNotificationData (WTF) -> PlatformNotificationData (STL) for
Blink to Chromium.
  - We need PlatformNotificationData (STL) -> WebNotificationData (STL) for
Chromium to Blink. (Since we receive the mojo call in //content.)

Then, when onion soup comes into play, we also need PlatformNotificationData
(STL) -> WebNotificationData (WTF).

I quite like that idea - thanks! I will have to spend some time figuring out
whether the Mojo bindings like having asymmetrical conversions, so I would like
to do that in a separate patch if that's OK with you.

[shimazu, 2017-02-13 00:38:56]

Probably you failed to upload the second patch.
Could you notice me again when you upload it?

https://codereview.chromium.org/2682353002/diff/1/content/renderer/service_wo...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2682353002/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.cc:1072:
ToWebNotificationData(notification_data), action_index, web_reply);
On 2017/02/10 16:41:06, Peter Beverloo wrote:
> On 2017/02/10 08:09:29, shimazu wrote:
> > I think you can use blink.mojom.Notification directly instead of
> > ToWebNotificationData. Using PlatformNotificationData is really
> straightforward,
> > but I'd like to have less [Native]s and like to remove _messages.h.
> > ConsoleMessage might be a good reference for type-mapping to a blink type.
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/console_me...
> > 
> > Creating another patch to introduce the typemap is also possible.
> > WDYT?
> 
> This relates to the layering issue that we spoke about last week.
> 
> Keeping WebNotificationData as the type used in Blink is an interesting idea.
> The type map configuration would be a bit tricky due to the mojo variants;
> 
>   - We need WebNotificationData (WTF) -> PlatformNotificationData (STL) for
> Blink to Chromium.
>   - We need PlatformNotificationData (STL) -> WebNotificationData (STL) for
> Chromium to Blink. (Since we receive the mojo call in //content.)
> 
> Then, when onion soup comes into play, we also need PlatformNotificationData
> (STL) -> WebNotificationData (WTF).
> 
> I quite like that idea - thanks! I will have to spend some time figuring out
> whether the Mojo bindings like having asymmetrical conversions, so I would
like
> to do that in a separate patch if that's OK with you.

Ah, sorry, I didn't understand accurately what you mentioned. The point here is
NotificationDatabaseData which is a container used under browser/ contains
PlatformNotificationData, so changing the type of argument for
DispatchNotificationClickEvent() doesn't seem reasonable.
In that case, using [Native] makes sense. 

Thanks for your kind explanation! :)

[awdf, 2017-02-13 07:04:42]

lgtm

https://codereview.chromium.org/2682353002/diff/1/content/browser/notificatio...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/2682353002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_event_dispatcher_impl.cc:212:
base::Optional<base::string16> optional_reply;
Is Optional<string> preferable to NullableString in general or just necessary
for mojo conversions? I'm curious.

[Peter Beverloo, 2017-02-13 14:14:33]

shimazu@ - sorry!! done now, PTAL
awdf@ - thanks!

https://codereview.chromium.org/2682353002/diff/1/content/browser/notificatio...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/2682353002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_event_dispatcher_impl.cc:212:
base::Optional<base::string16> optional_reply;
On 2017/02/13 07:04:42, awdf wrote:
> Is Optional<string> preferable to NullableString in general or just necessary
> for mojo conversions? I'm curious.

It's necessary for Mojo, but since base::Optional is a generalized container
based on an upcoming STL type I'd say that it's preferred going forward.

[Peter Beverloo, 2017-02-13 14:14:47]

The CQ bit was checked by peter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 14:15:03]

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

[Peter Beverloo, 2017-02-13 14:15:40]

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

[Peter Beverloo, 2017-02-13 14:15:41]

Also +tsepez for IPC->Mojo conversion

[shimazu, 2017-02-13 14:24:49]

lgtm

[commit-bot: I haz the power, 2017-02-13 15:40:49]

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

[commit-bot: I haz the power, 2017-02-13 15:40:50]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2017-02-13 17:59:34]

lgtm

[Peter Beverloo, 2017-02-13 18:01:04]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-02-13 18:01:04]

The patchset sent to the CQ was uploaded after l-g-t-m from awdf@chromium.org
Link to the patchset: https://codereview.chromium.org/2682353002/#ps20001
(title: "Mojoify the notificationclick and notificationclose events")

[commit-bot: I haz the power, 2017-02-13 18:02: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, 2017-02-13 18:08:41]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487008864404970,
"parent_rev": "66f70447e561ce2d850be2e2ef4d1a1be4364ac7", "commit_rev":
"f8ea4c45d43e8e4871f9a51f74fa8d74618c0886"}

[commit-bot: I haz the power, 2017-02-13 18:09:16]

Description was changed from

==========
Mojoify the notificationclick and notificationclose events

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

to

==========
Mojoify the notificationclick and notificationclose events

BUG=595685

Review-Url: https://codereview.chromium.org/2682353002
Cr-Commit-Position: refs/heads/master@{#450007}
Committed:
https://chromium.googlesource.com/chromium/src/+/f8ea4c45d43e8e4871f9a51f74fa...
==========

[commit-bot: I haz the power, 2017-02-13 18:09:17]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/f8ea4c45d43e8e4871f9a51f74fa...