Notifications: Improve SW notification property layout test

Notifications: Improve SW notification property layout test

This generalizes serviceworker-notificationclick-event-reflection.html
to also check that the properties are correctly reflected by
ServiceWorkerRegistration.getNotifications().

Hence the test is renamed to serviceworker-notification-properties.html

This will be useful for testing notification actions, since the
getNotifications test operates on real Notification objects rather than
cloned ones, so it'll be possible to test immutability etc.

BUG=513671
Committed: https://crrev.com/55ad68223578c93a82538f50c67ab23680c9d72b
Cr-Commit-Position: refs/heads/master@{#355309}

[johnme, 2015-10-21 13:25:47]

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

[Peter Beverloo, 2015-10-21 13:29:00]

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

https://codereview.chromium.org/1417913002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:69:
// set on the (cloned) Notification object are as expected.
no need for parenthesis

https://codereview.chromium.org/1417913002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:81:
// (non-cloned) Notification object from getNotifications.
no need for parenthesis

https://codereview.chromium.org/1417913002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:85:
Object.keys(options).forEach(function(key) {
Can we extract this to a function w/ an argument to indicate the context in the
assert's description, to avoid duplication?

[johnme, 2015-10-21 14:21:18]

Addressed review comments - thanks :)

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

https://codereview.chromium.org/1417913002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:69:
// set on the (cloned) Notification object are as expected.
On 2015/10/21 13:29:00, Peter Beverloo wrote:
> no need for parenthesis

Done.

https://codereview.chromium.org/1417913002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:81:
// (non-cloned) Notification object from getNotifications.
On 2015/10/21 13:29:00, Peter Beverloo wrote:
> no need for parenthesis

Done.

https://codereview.chromium.org/1417913002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:85:
Object.keys(options).forEach(function(key) {
On 2015/10/21 13:29:00, Peter Beverloo wrote:
> Can we extract this to a function w/ an argument to indicate the context in
the
> assert's description, to avoid duplication?

Done.

[Peter Beverloo, 2015-10-21 14:26:35]

lgtm

https://codereview.chromium.org/1417913002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html
(right):

https://codereview.chromium.org/1417913002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:18:
description = description || '';
nit: remove this please, it's unused

https://codereview.chromium.org/1417913002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:21:
assert_object_equals(actual[key], expected[key], description + ' [field ' + key
+ ']');
nit: perhaps store the concatenated string in a local?

https://codereview.chromium.org/1417913002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:65:
options.actions = options.actions.slice(0, Notification.maxActions);
Redefining maxActions might be a fun thing to test :)

[johnme, 2015-10-21 15:55:35]

Addressed review nits - thanks :)

https://codereview.chromium.org/1417913002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html
(right):

https://codereview.chromium.org/1417913002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:18:
description = description || '';
On 2015/10/21 14:26:35, Peter Beverloo wrote:
> nit: remove this please, it's unused

Done.

https://codereview.chromium.org/1417913002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:21:
assert_object_equals(actual[key], expected[key], description + ' [field ' + key
+ ']');
On 2015/10/21 14:26:34, Peter Beverloo wrote:
> nit: perhaps store the concatenated string in a local?

Done.

https://codereview.chromium.org/1417913002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:65:
options.actions = options.actions.slice(0, Notification.maxActions);
On 2015/10/21 14:26:34, Peter Beverloo wrote:
> Redefining maxActions might be a fun thing to test :)

Done (added test to notification-properties.html).

[johnme, 2015-10-21 15:55:39]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-10-21 15:55:39]

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/1417913002/#ps40001
(title: "Addressed review nits")

[commit-bot: I haz the power, 2015-10-21 15:55:55]

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

[commit-bot: I haz the power, 2015-10-21 16:33:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-21 16:34:42]

Patchset 3 (id:??) landed as
https://crrev.com/55ad68223578c93a82538f50c67ab23680c9d72b
Cr-Commit-Position: refs/heads/master@{#355309}