Add NotificationOptions.actions so websites can provide buttons (blink)

Add NotificationOptions.actions so websites can provide buttons (blink)

Spec: https://github.com/whatwg/notifications/pull/48

Intent to implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/6tlbJW0NjEA/LF0kPfJx7c0J

This patch only includes NotificationAction.title; NotificationAction.action
is added by the subsequent patch: https://codereview.chromium.org/1263043003

BUG=513671
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199893

[johnme, 2015-07-30 15:49:51]

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

[Peter Beverloo, 2015-07-30 16:24:08]

Cool, thanks John! Please link to both the specification and the Intent to
Implement in your CL description.

https://codereview.chromium.org/1263043002/diff/1/LayoutTests/http/tests/noti...
File LayoutTests/http/tests/notifications/notification-properties.html (right):

https://codereview.chromium.org/1263043002/diff/1/LayoutTests/http/tests/noti...
LayoutTests/http/tests/notifications/notification-properties.html:36:
assert_object_equals(notification.actions, options.actions.slice(0, 2));
Please do not assume "2", and actually check Notification.maxActions. Embedders
are free to change this.

https://codereview.chromium.org/1263043002/diff/1/LayoutTests/http/tests/noti...
LayoutTests/http/tests/notifications/notification-properties.html:36:
assert_object_equals(notification.actions, options.actions.slice(0, 2));
Could you add a line testing that we can modify notification.actions? (This will
make sure that the test breaks when we introduce FrozenArray<>.)

https://codereview.chromium.org/1263043002/diff/1/LayoutTests/http/tests/noti...
File
LayoutTests/http/tests/notifications/serviceworker-notificationclick-event-reflection.html
(right):

https://codereview.chromium.org/1263043002/diff/1/LayoutTests/http/tests/noti...
LayoutTests/http/tests/notifications/serviceworker-notificationclick-event-reflection.html:72:
assert_equals(event.data.notification.actions.length, 0);
This doesn't look like a great idea. Serializing actions is a concern for the
embedder, so the patch to fix this will be in content/, while the test lives in
Blink. (You'd need a three-sided patch.)

https://codereview.chromium.org/1263043002/diff/1/LayoutTests/http/tests/noti...
File
LayoutTests/http/tests/notifications/serviceworkerregistration-document-actions-throw.html
(right):

https://codereview.chromium.org/1263043002/diff/1/LayoutTests/http/tests/noti...
LayoutTests/http/tests/notifications/serviceworkerregistration-document-actions-throw.html:12:
var scope = 'resources/scope/' + location.pathname,
Could you add a description, and scope |scope|/|script| to the async_test()?
(like all other tests in this suite)

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
Source/modules/notifications/Notification.cpp:144:
notification->m_vibrate.appendRange(data.vibrate.begin(), data.vibrate.end());
Please don't do this. Everything uses setters and here you access the property
directly. (Consistency)

We need to change this entirely - gone with the setters altogether by doing this
*in* the instance rather than in the ::create() method. Refactorings are due
that will address this, but we shouldn't break consistency until we do.

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
Source/modules/notifications/Notification.cpp:152:
webActionsToActions(data.actions, notification->m_actions);
Dito.

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
Source/modules/notifications/Notification.h:112: static void
webActionsToActions(const WebVector<WebNotificationAction>& webActions,
Vector<NotificationAction>& actions);
This can be private.

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
Source/modules/notifications/Notification.h:130: static const size_t maxActions
= 2;
This is an embedder concern, not a Blink one. Perhaps add something like
WebNotificationManager::supportedActionCount()?

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
Source/modules/notifications/Notification.h:189: void
Notification::actionsToWebActions(const Vector<NotificationAction, C, A>&
actions, WebVector<WebNotificationAction>& webActions)
Why do we need template<>? This should be defined in the implementation file,
and the |maxActions| constant should be local to Notification.cpp.

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
Source/modules/notifications/Notification.h:189: void
Notification::actionsToWebActions(const Vector<NotificationAction, C, A>&
actions, WebVector<WebNotificationAction>& webActions)
nit: While the Blink style allows it, I'd prefer it if you would pass the second
argument as a pointer to indicate that it's getting changed. Clearer when
reading the call-sites.

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
File Source/modules/notifications/Notification.idl (right):

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
Source/modules/notifications/Notification.idl:59: // TODO(johnme): Make this a
FrozenArray instead before shipping.
Bug?

https://codereview.chromium.org/1263043002/diff/1/public/platform/modules/not...
File public/platform/modules/notifications/WebNotificationAction.h (right):

https://codereview.chromium.org/1263043002/diff/1/public/platform/modules/not...
public/platform/modules/notifications/WebNotificationAction.h:12: // Structure
representing the data associated with a Web Notification action.
Could you annotate that the empty and copy-constructors exist because it's used
in vectors?

[johnme, 2015-07-31 15:10:10]

Addressed review comments - thanks!

https://codereview.chromium.org/1263043002/diff/1/LayoutTests/http/tests/noti...
File LayoutTests/http/tests/notifications/notification-properties.html (right):

https://codereview.chromium.org/1263043002/diff/1/LayoutTests/http/tests/noti...
LayoutTests/http/tests/notifications/notification-properties.html:36:
assert_object_equals(notification.actions, options.actions.slice(0, 2));
On 2015/07/30 16:24:07, Peter Beverloo wrote:
> Please do not assume "2", and actually check Notification.maxActions.
Embedders
> are free to change this.

Done.

https://codereview.chromium.org/1263043002/diff/1/LayoutTests/http/tests/noti...
LayoutTests/http/tests/notifications/notification-properties.html:36:
assert_object_equals(notification.actions, options.actions.slice(0, 2));
On 2015/07/30 16:24:07, Peter Beverloo wrote:
> Could you add a line testing that we can modify notification.actions? (This
will
> make sure that the test breaks when we introduce FrozenArray<>.)

Actually, no, we can't modify notification.actions. So we seem to match the
spec's deeply FrozenArray requirement already (though possibly due to a wider
bindings IDL compliance issue). I added a test that it is immutable instead.

https://codereview.chromium.org/1263043002/diff/1/LayoutTests/http/tests/noti...
File
LayoutTests/http/tests/notifications/serviceworker-notificationclick-event-reflection.html
(right):

https://codereview.chromium.org/1263043002/diff/1/LayoutTests/http/tests/noti...
LayoutTests/http/tests/notifications/serviceworker-notificationclick-event-reflection.html:72:
assert_equals(event.data.notification.actions.length, 0);
On 2015/07/30 16:24:07, Peter Beverloo wrote:
> This doesn't look like a great idea. Serializing actions is a concern for the
> embedder, so the patch to fix this will be in content/, while the test lives
in
> Blink. (You'd need a three-sided patch.)

Ok, I removed the assert and now just return (with TODO).

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
Source/modules/notifications/Notification.cpp:144:
notification->m_vibrate.appendRange(data.vibrate.begin(), data.vibrate.end());
On 2015/07/30 16:24:07, Peter Beverloo wrote:
> Please don't do this. Everything uses setters and here you access the property
> directly. (Consistency)
> 
> We need to change this entirely - gone with the setters altogether by doing
this
> *in* the instance rather than in the ::create() method. Refactorings are due
> that will address this, but we shouldn't break consistency until we do.

Done.

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
Source/modules/notifications/Notification.cpp:152:
webActionsToActions(data.actions, notification->m_actions);
On 2015/07/30 16:24:07, Peter Beverloo wrote:
> Dito.

Done.

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
Source/modules/notifications/Notification.h:112: static void
webActionsToActions(const WebVector<WebNotificationAction>& webActions,
Vector<NotificationAction>& actions);
On 2015/07/30 16:24:07, Peter Beverloo wrote:
> This can be private.

Done.

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
Source/modules/notifications/Notification.h:130: static const size_t maxActions
= 2;
On 2015/07/30 16:24:08, Peter Beverloo wrote:
> This is an embedder concern, not a Blink one. Perhaps add something like
> WebNotificationManager::supportedActionCount()?

Done.

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
Source/modules/notifications/Notification.h:189: void
Notification::actionsToWebActions(const Vector<NotificationAction, C, A>&
actions, WebVector<WebNotificationAction>& webActions)
On 2015/07/30 16:24:07, Peter Beverloo wrote:
> nit: While the Blink style allows it, I'd prefer it if you would pass the
second
> argument as a pointer to indicate that it's getting changed. Clearer when
> reading the call-sites.

Done.

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
Source/modules/notifications/Notification.h:189: void
Notification::actionsToWebActions(const Vector<NotificationAction, C, A>&
actions, WebVector<WebNotificationAction>& webActions)
On 2015/07/30 16:24:08, Peter Beverloo wrote:
> Why do we need template<>? This should be defined in the implementation file,
> and the |maxActions| constant should be local to Notification.cpp.

Done (without the template, we need an extra copy from HeapVector to Vector, but
we discussed offline and agreed that was ok).

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
File Source/modules/notifications/Notification.idl (right):

https://codereview.chromium.org/1263043002/diff/1/Source/modules/notification...
Source/modules/notifications/Notification.idl:59: // TODO(johnme): Make this a
FrozenArray instead before shipping.
On 2015/07/30 16:24:08, Peter Beverloo wrote:
> Bug?

No, but removed and tweaked TODO below since experimentally sequence seems to
behave as a deeply frozen FrozenArray already :-s

https://codereview.chromium.org/1263043002/diff/1/public/platform/modules/not...
File public/platform/modules/notifications/WebNotificationAction.h (right):

https://codereview.chromium.org/1263043002/diff/1/public/platform/modules/not...
public/platform/modules/notifications/WebNotificationAction.h:12: // Structure
representing the data associated with a Web Notification action.
On 2015/07/30 16:24:08, Peter Beverloo wrote:
> Could you annotate that the empty and copy-constructors exist because it's
used
> in vectors?

Done.

[Peter Beverloo, 2015-07-31 15:38:49]

lgtm

https://codereview.chromium.org/1263043002/diff/40001/Source/modules/notifica...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1263043002/diff/40001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:117:
actions.appendRange(options.actions().begin(), options.actions().begin() +
std::min(notificationManager()->maxActions(), options.actions().size()));
nit: you can just write |maxActions()| rather than
|notificationManager()->maxActions()|.

https://codereview.chromium.org/1263043002/diff/40001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:321: size_t count =
std::min(notificationManager()->maxActions(), actions.size());
dito

https://codereview.chromium.org/1263043002/diff/40001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:324: for (size_t i = 0; i < count;
++i) {
nit: no need for {} in one-line if statements

https://codereview.chromium.org/1263043002/diff/40001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:334:
(*actions)[i].setTitle(webActions[i].title);
dito

https://codereview.chromium.org/1263043002/diff/40001/Source/modules/notifica...
File Source/modules/notifications/Notification.idl (right):

https://codereview.chromium.org/1263043002/diff/40001/Source/modules/notifica...
Source/modules/notifications/Notification.idl:65: // TODO(johnme): The spec uses
a FrozenArray, but sequence seems to behave like one already. Investigate why.
Please refer to an Issue #. Figuring this out is a ship blocker.

https://codereview.chromium.org/1263043002/diff/40001/Source/modules/notifica...
File Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/1263043002/diff/40001/Source/modules/notifica...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:76: for
(NotificationAction action : options.actions()) {
const&

[johnme, 2015-07-31 16:08:52]

Addressed review comments - thanks!

https://codereview.chromium.org/1263043002/diff/1/LayoutTests/http/tests/noti...
File
LayoutTests/http/tests/notifications/serviceworkerregistration-document-actions-throw.html
(right):

https://codereview.chromium.org/1263043002/diff/1/LayoutTests/http/tests/noti...
LayoutTests/http/tests/notifications/serviceworkerregistration-document-actions-throw.html:12:
var scope = 'resources/scope/' + location.pathname,
On 2015/07/30 16:24:07, Peter Beverloo wrote:
> Could you add a description, and scope |scope|/|script| to the async_test()?
> (like all other tests in this suite)

Done.

https://codereview.chromium.org/1263043002/diff/40001/Source/modules/notifica...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1263043002/diff/40001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:117:
actions.appendRange(options.actions().begin(), options.actions().begin() +
std::min(notificationManager()->maxActions(), options.actions().size()));
On 2015/07/31 15:38:49, Peter Beverloo wrote:
> nit: you can just write |maxActions()| rather than
> |notificationManager()->maxActions()|.

Done.

https://codereview.chromium.org/1263043002/diff/40001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:321: size_t count =
std::min(notificationManager()->maxActions(), actions.size());
On 2015/07/31 15:38:49, Peter Beverloo wrote:
> dito

Done.

https://codereview.chromium.org/1263043002/diff/40001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:324: for (size_t i = 0; i < count;
++i) {
On 2015/07/31 15:38:49, Peter Beverloo wrote:
> nit: no need for {} in one-line if statements

Done (but this becomes 2-line in https://codereview.chromium.org/1263043003).

https://codereview.chromium.org/1263043002/diff/40001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:334:
(*actions)[i].setTitle(webActions[i].title);
On 2015/07/31 15:38:49, Peter Beverloo wrote:
> dito

Done (but this becomes 2-line in https://codereview.chromium.org/1263043003).

https://codereview.chromium.org/1263043002/diff/40001/Source/modules/notifica...
File Source/modules/notifications/Notification.idl (right):

https://codereview.chromium.org/1263043002/diff/40001/Source/modules/notifica...
Source/modules/notifications/Notification.idl:65: // TODO(johnme): The spec uses
a FrozenArray, but sequence seems to behave like one already. Investigate why.
On 2015/07/31 15:38:49, Peter Beverloo wrote:
> Please refer to an Issue #. Figuring this out is a ship blocker.

Done.

https://codereview.chromium.org/1263043002/diff/40001/Source/modules/notifica...
File Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/1263043002/diff/40001/Source/modules/notifica...
Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:76: for
(NotificationAction action : options.actions()) {
On 2015/07/31 15:38:49, Peter Beverloo wrote:
> const&

Done.

[johnme, 2015-07-31 16:10:12]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-07-31 16:10:12]

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/1263043002/#ps60001
(title: "Address additional review comments")

[commit-bot: I haz the power, 2015-07-31 16:11:01]

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

[commit-bot: I haz the power, 2015-07-31 16:14:09]

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

[commit-bot: I haz the power, 2015-07-31 16:14:10]

Try jobs failed on following builders:
  android_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)
  linux_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_compile_dbg/...)
  linux_chromium_gn_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)

[johnme, 2015-07-31 16:23:10]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-07-31 16:23:10]

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/1263043002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2015-07-31 16:23:50]

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

[commit-bot: I haz the power, 2015-07-31 16:27:08]

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

[commit-bot: I haz the power, 2015-07-31 16:27:09]

Try jobs failed on following builders:
  android_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)
  linux_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_compile_dbg/...)
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)
  linux_chromium_gn_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)

[johnme, 2015-07-31 16:36:47]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-07-31 16:36:47]

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/1263043002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-07-31 16:37:08]

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

[commit-bot: I haz the power, 2015-07-31 17:53:40]

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

[commit-bot: I haz the power, 2015-07-31 17:53:41]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/72235)

[johnme, 2015-07-31 18:11:40]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-07-31 18:11:40]

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/1263043002/#ps120001
(title: "Add to global-interface-listing*-expected.txt")

[commit-bot: I haz the power, 2015-07-31 18:11:49]

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

[commit-bot: I haz the power, 2015-07-31 20:24:30]

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

[commit-bot: I haz the power, 2015-07-31 20:24:31]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/72241)

[johnme, 2015-08-03 09:59:13]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2015-08-03 09:59:22]

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

[commit-bot: I haz the power, 2015-08-03 11:20:49]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199893