Only allow action buttons in persistent notifications per spec update

Only allow action buttons in persistent notifications per spec update

See https://github.com/whatwg/notifications/pull/48#issuecomment-148123079

BUG=513671
Committed: https://crrev.com/0749528e3f8928fe591879705e891324750bcd7d
Cr-Commit-Position: refs/heads/master@{#355859}

[johnme, 2015-10-21 16:48:51]

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

[Peter Beverloo, 2015-10-21 17:02:52]

lgtm

https://codereview.chromium.org/1409143004/diff/1/content/shell/browser/layou...
File content/shell/browser/layout_test/layout_test_notification_manager.cc
(right):

https://codereview.chromium.org/1409143004/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/layout_test_notification_manager.cc:116:
DCHECK_EQ(action_index, -1) << "Action buttons are only supported for "
Can we have a constant please? That also removes the need to have the comment.

https://codereview.chromium.org/1409143004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/notifications/notification-properties.html
(right):

https://codereview.chromium.org/1409143004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/notifications/notification-properties.html:102:
// Notification actions should only be supported for persistent notifications
nit: sentences end with periods :)

https://codereview.chromium.org/1409143004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html
(right):

https://codereview.chromium.org/1409143004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:54:
action: "" + i,
nit: no need for the "" (ES ToString() will be applied)

https://codereview.chromium.org/1409143004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:100:
if (notifications[0].actions.length) {
When would this be true?

https://codereview.chromium.org/1409143004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:106:
// TODO(johnme): This should pass before shipping Notification.actions.
+bug

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

https://codereview.chromium.org/1409143004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/Notification.cpp:82: if
(!options.actions().isEmpty()) {
It's unfortunate that the place in the algorithm you chose to insert the
`actions` steps in makes it hard or us to support it in order.

Per the spec:
  (1) Check for `silent` & `vibrate`/`sound`, throw if invalid.
  (2) Check for `renotify` & `tag`, throw if invalid.
  (3) Check for `actions` and persistent, throw if invalid.
  ...

Implementation:
  (1) Check for `action` and persistent, throw if invalid.
  (2) Check for `silent` & `vibrate`/`sound`, throw if invalid.
  ...

This is Web-observable behaviour and could cause discrepancies between
implementations, but I'm not sure how much to care. It's certainly not worth
giving createWebNotificationData knowledge of the type of notification.

[johnme, 2015-10-21 18:11:31]

Addressed review comments - thanks :)

https://codereview.chromium.org/1409143004/diff/1/content/shell/browser/layou...
File content/shell/browser/layout_test/layout_test_notification_manager.cc
(right):

https://codereview.chromium.org/1409143004/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/layout_test_notification_manager.cc:116:
DCHECK_EQ(action_index, -1) << "Action buttons are only supported for "
On 2015/10/21 17:02:52, Peter Beverloo wrote:
> Can we have a constant please? That also removes the need to have the comment.

I'll add the constant in a follow-up patch to avoid ballooning this one (I'll
still need the comment though, to explain why the DCHECK is there).

https://codereview.chromium.org/1409143004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/notifications/notification-properties.html
(right):

https://codereview.chromium.org/1409143004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/notifications/notification-properties.html:102:
// Notification actions should only be supported for persistent notifications
On 2015/10/21 17:02:52, Peter Beverloo wrote:
> nit: sentences end with periods :)

Done.

https://codereview.chromium.org/1409143004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html
(right):

https://codereview.chromium.org/1409143004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:54:
action: "" + i,
On 2015/10/21 17:02:52, Peter Beverloo wrote:
> nit: no need for the "" (ES ToString() will be applied)

I prefer to be explicit though. Changed it to `"a" + i` so it's no longer
redundant.

https://codereview.chromium.org/1409143004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:100:
if (notifications[0].actions.length) {
On 2015/10/21 17:02:52, Peter Beverloo wrote:
> When would this be true?

It'll be true whenever Notification.maxActions > 0. That's currently always
true, but in general the actions tests have avoided hardcoding specific values
for maxActions, since Chrome may change this freely later.

https://codereview.chromium.org/1409143004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:106:
// TODO(johnme): This should pass before shipping Notification.actions.
On 2015/10/21 17:02:52, Peter Beverloo wrote:
> +bug

Done.

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

https://codereview.chromium.org/1409143004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/Notification.cpp:82: if
(!options.actions().isEmpty()) {
On 2015/10/21 17:02:52, Peter Beverloo wrote:
> It's unfortunate that the place in the algorithm you chose to insert the
> `actions` steps in makes it hard or us to support it in order.
> 
> Per the spec:
>   (1) Check for `silent` & `vibrate`/`sound`, throw if invalid.
>   (2) Check for `renotify` & `tag`, throw if invalid.
>   (3) Check for `actions` and persistent, throw if invalid.
>   ...
> 
> Implementation:
>   (1) Check for `action` and persistent, throw if invalid.
>   (2) Check for `silent` & `vibrate`/`sound`, throw if invalid.
>   ...
> 
> This is Web-observable behaviour and could cause discrepancies between
> implementations, but I'm not sure how much to care. It's certainly not worth
> giving createWebNotificationData knowledge of the type of notification.

Hmm yeah, and we can't even move this check after createWebNotificationData
because then it would incorrectly occur after the "Set notification’s data to a
structured clone of options’s data. Rethrow any exceptions." step.

I've updated my spec patch to re-order these.

[johnme, 2015-10-21 18:12:29]

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

[johnme, 2015-10-21 18:12:30]

avi@chromium.org: Please rubber stamp
content/shell/browser/layout_test/layout_test_notification_manager.cc - thanks
:)

[johnme, 2015-10-23 17:59:06]

On 2015/10/21 18:12:30, johnme wrote:
> mailto:avi@chromium.org: Please rubber stamp
> content/shell/browser/layout_test/layout_test_notification_manager.cc - thanks
> :)

Peter is now an owner of content/shell/, so I'll go ahead and land this :)

[johnme, 2015-10-23 17:59:09]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-10-23 17:59: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/1409143004/#ps20001
(title: "Address review nits")

[commit-bot: I haz the power, 2015-10-23 18:00:27]

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

[commit-bot: I haz the power, 2015-10-23 19:33:08]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-23 19:33:57]

Patchset 2 (id:??) landed as
https://crrev.com/0749528e3f8928fe591879705e891324750bcd7d
Cr-Commit-Position: refs/heads/master@{#355859}