- Changed instrumentation-service-worker.js so the value NaN can be tested

- Changed instrumentation-service-worker.js so the value NaN can be copied properly in cloneNotification.
- Added testing of NaN and null in serviceworker-notificationclick-event-data-reflection.html

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

[lwchkg, 2015-09-01 15:09:59]

lwchkg@gmail.com changed reviewers:
+ peter@chromium.org

[lwchkg, 2015-09-01 15:13:59]

lwchkg@gmail.com changed reviewers:
+ johnme@chromium.org

[lwchkg, 2015-09-01 15:14:53]

Dear all,

   Please have a look. Thanks.

Regards,
WC Leung.

[Peter Beverloo, 2015-09-01 15:31:44]

I've got a few minor nits - thanks for doing this!!

https://codereview.chromium.org/1311413006/diff/1/LayoutTests/http/tests/noti...
File
LayoutTests/http/tests/notifications/resources/instrumentation-service-worker.js
(right):

https://codereview.chromium.org/1311413006/diff/1/LayoutTests/http/tests/noti...
LayoutTests/http/tests/notifications/resources/instrumentation-service-worker.js:8:
if (typeof src != 'object')
nit: could change this to the following, which matches the branch in
serviceworker-notificationclick-event-data-reflection.html more closely.

(typeof src !== 'object' || src === null)

(And remove lines 10-11.)

https://codereview.chromium.org/1311413006/diff/1/LayoutTests/http/tests/noti...
LayoutTests/http/tests/notifications/resources/instrumentation-service-worker.js:14:
if (src.hasOwnProperty(property)) {
nit: no need for {} (one-statement if clause)

https://codereview.chromium.org/1311413006/diff/1/LayoutTests/http/tests/noti...
LayoutTests/http/tests/notifications/resources/instrumentation-service-worker.js:23:
var dst = JSON.parse(stringifyDOMObject(notification));
nit: could we name this "copiedNotification"? We prefer appropriately named
variable over the shorter convenient ones in Blink.

(Although I think "src" in cloneObject() is clear enough, sorry for the
inconsistency :-).)

[lwchkg, 2015-09-01 16:21:09]

Uploaded another patch to address the comments. Thanks for your quick review!

https://codereview.chromium.org/1311413006/diff/1/LayoutTests/http/tests/noti...
File
LayoutTests/http/tests/notifications/resources/instrumentation-service-worker.js
(right):

https://codereview.chromium.org/1311413006/diff/1/LayoutTests/http/tests/noti...
LayoutTests/http/tests/notifications/resources/instrumentation-service-worker.js:8:
if (typeof src != 'object')
On 2015/09/01 15:31:44, Peter Beverloo wrote:
> nit: could change this to the following, which matches the branch in
> serviceworker-notificationclick-event-data-reflection.html more closely.
> 
> (typeof src !== 'object' || src === null)
> 
> (And remove lines 10-11.)

Sounds good.

https://codereview.chromium.org/1311413006/diff/1/LayoutTests/http/tests/noti...
LayoutTests/http/tests/notifications/resources/instrumentation-service-worker.js:14:
if (src.hasOwnProperty(property)) {
On 2015/09/01 15:31:44, Peter Beverloo wrote:
> nit: no need for {} (one-statement if clause)

Acknowledged.

https://codereview.chromium.org/1311413006/diff/1/LayoutTests/http/tests/noti...
LayoutTests/http/tests/notifications/resources/instrumentation-service-worker.js:23:
var dst = JSON.parse(stringifyDOMObject(notification));
On 2015/09/01 15:31:44, Peter Beverloo wrote:
> nit: could we name this "copiedNotification"? We prefer appropriately named
> variable over the shorter convenient ones in Blink.
> 
> (Although I think "src" in cloneObject() is clear enough, sorry for the
> inconsistency :-).)

Sounds good. Agree that "dst" is not clear enough. Actually the string "dst" was
copied from harness-helper.js, and it took me at least a few seconds to
recognize the pattern.

[Peter Beverloo, 2015-09-01 16:38:21]

lgtm, thanks!

[Peter Beverloo, 2015-09-01 16:38:37]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2015-09-01 16:38:54]

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

[commit-bot: I haz the power, 2015-09-01 17:23:44]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201562