Add layout test for reply property of notificationclick event

Add layout test for reply property of notificationclick event

BUG=599859
Committed: https://crrev.com/f8bdbe8a21ba9a9a62755a5aba12770f97234613
Cr-Commit-Position: refs/heads/master@{#425281}

[awdf, 2016-10-10 17:37:16]

Patchset #1 (id:1) has been deleted

[awdf, 2016-10-10 17:37:22]

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

[commit-bot: I haz the power, 2016-10-10 17:37:43]

Dry run: 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, 2016-10-10 18:52:49]

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

[commit-bot: I haz the power, 2016-10-10 18:52:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[awdf, 2016-10-11 10:56:39]

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

[commit-bot: I haz the power, 2016-10-11 10:56:50]

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

[awdf, 2016-10-11 11:42:52]

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

[awdf, 2016-10-11 11:43:00]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-10-11 11:43:05]

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

[awdf, 2016-10-11 11:43:19]

Patchset #1 (id:40001) has been deleted

[awdf, 2016-10-11 11:51:29]

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

[awdf, 2016-10-11 11:51:29]

As proposed in the other review, here's a layout test that calls
simulateWebNotificationClickWithReply, which unifies with
simulateWebNotificationClick at the c++ level. (Doing it this way instead of
solely using simulateWebNotificationClick with another parameter seemed to
involve writing the least code, let me know if you think it's confusing though)

[commit-bot: I haz the power, 2016-10-11 11:52:36]

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

[commit-bot: I haz the power, 2016-10-11 11:52:37]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Peter Beverloo, 2016-10-11 14:28:40]

https://codereview.chromium.org/2403893002/diff/60001/components/test_runner/...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/2403893002/diff/60001/components/test_runner/...
components/test_runner/test_runner.cc:1368: void
TestRunnerBindings::SimulateWebNotificationClickWithReply(
Ah, yeah, this is a fine middle ground :-).

Depending on whether you agree on the base::NullableString16 comment in
the other CL, we probably want TestRunner::SimulateWebNotificationClick()'s
|reply| to argument to be of that type too?

https://codereview.chromium.org/2403893002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notificationclick-event-reply-reflection.html
(right):

https://codereview.chromium.org/2403893002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notificationclick-event-reply-reflection.html:47:
assert_equals(event.data.reply, 'My reply.');
We should probably test the NULL-string and empty-string cases too?

[awdf, 2016-10-12 14:28:37]

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

[commit-bot: I haz the power, 2016-10-12 14:28:43]

Dry run: 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, 2016-10-12 14:38:16]

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

[commit-bot: I haz the power, 2016-10-12 14:38:17]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[awdf, 2016-10-12 14:59:45]

https://codereview.chromium.org/2403893002/diff/60001/components/test_runner/...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/2403893002/diff/60001/components/test_runner/...
components/test_runner/test_runner.cc:1368: void
TestRunnerBindings::SimulateWebNotificationClickWithReply(
On 2016/10/11 14:28:39, Peter Beverloo wrote:
> Ah, yeah, this is a fine middle ground :-).
> 
> Depending on whether you agree on the base::NullableString16 comment in
> the other CL, we probably want TestRunner::SimulateWebNotificationClick()'s
> |reply| to argument to be of that type too?

Done.

https://codereview.chromium.org/2403893002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notificationclick-event-reply-reflection.html
(right):

https://codereview.chromium.org/2403893002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notificationclick-event-reply-reflection.html:47:
assert_equals(event.data.reply, 'My reply.');
On 2016/10/11 14:28:39, Peter Beverloo wrote:
> We should probably test the NULL-string and empty-string cases too?

Done the empty string case but not sure how to check the NULL case - tried two
things but neither test gave the expected result of null
1. Call simulateWebNotificationClickWithReply but pass null instead of a string
- right now it will just error out because I've made that particular test
binding expect a string - should I change that?
2. Call simulateWebNotificationClick and then check reply is null - but it's the
empty string and I'm not sure why. Is it because of the eventInit constructor,
is this a place where it's used?

[Peter Beverloo, 2016-10-12 16:39:07]

lgtm % two additional tests

> Done the empty string case but not sure how to check the NULL case -
> tried two things but neither test gave the expected result of null
> 1. Call simulateWebNotificationClickWithReply but pass null instead of a
> string - right now it will just error out because I've made that
> particular test binding expect a string - should I change that?

No, because simulateWebNotificationClick() describes the "without click"
case, so that should be NULL. simulateWebNotificationClickWithReply()
describes the "the user had the ability to enter text" case. Would still
be good to check for the empty string.

> 2. Call simulateWebNotificationClick and then check reply is null - but
> it's the empty string and I'm not sure why. Is it because of the
> eventInit constructor, is this a place where it's used?

That's a bug! :) Let's have a look offline.

[awdf, 2016-10-13 11:57:19]

On 2016/10/12 16:39:07, Peter Beverloo wrote:
> lgtm % two additional tests
> 
> > Done the empty string case but not sure how to check the NULL case -
> > tried two things but neither test gave the expected result of null
> > 1. Call simulateWebNotificationClickWithReply but pass null instead of a
> > string - right now it will just error out because I've made that
> > particular test binding expect a string - should I change that?
> 
> No, because simulateWebNotificationClick() describes the "without click"
> case, so that should be NULL. simulateWebNotificationClickWithReply()
> describes the "the user had the ability to enter text" case. Would still
> be good to check for the empty string.
> 
> > 2. Call simulateWebNotificationClick and then check reply is null - but
> > it's the empty string and I'm not sure why. Is it because of the
> > eventInit constructor, is this a place where it's used?
> 
> That's a bug! :) Let's have a look offline.

It's null everywhere up to where it becomes converted from NullableString to
WebString, then if I log it (with WebString.latin1()) it prints an empty
string... Expected?

[awdf, 2016-10-13 13:17:29]

I'm up for getting rid of the confusing NotificationEventInit idl in a separate
CL - but won't that also require a change to the spec? (It's defined at
https://notifications.spec.whatwg.org/#dictdef-notificationeventinit )

https://codereview.chromium.org/2403893002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notificationclick-event-reply-reflection.html
(right):

https://codereview.chromium.org/2403893002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notificationclick-event-reply-reflection.html:47:
assert_equals(event.data.reply, 'My reply.');
On 2016/10/12 14:59:45, awdf wrote:
> On 2016/10/11 14:28:39, Peter Beverloo wrote:
> > We should probably test the NULL-string and empty-string cases too?
> 
> Done the empty string case but not sure how to check the NULL case - tried two
> things but neither test gave the expected result of null
> 1. Call simulateWebNotificationClickWithReply but pass null instead of a
string
> - right now it will just error out because I've made that particular test
> binding expect a string - should I change that?
> 2. Call simulateWebNotificationClick and then check reply is null - but it's
the
> empty string and I'm not sure why. Is it because of the eventInit constructor,
> is this a place where it's used?

Done.

[awdf, 2016-10-13 13:17:31]

I'm up for getting rid of the confusing NotificationEventInit idl in a separate
CL - but won't that also require a change to the spec? (It's defined at
https://notifications.spec.whatwg.org/#dictdef-notificationeventinit )

[awdf, 2016-10-13 14:44:07]

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

[commit-bot: I haz the power, 2016-10-13 14:44:26]

Dry run: 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, 2016-10-13 16:27:09]

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

[commit-bot: I haz the power, 2016-10-13 16:27:10]

Dry run: This issue passed the CQ dry run.

[awdf, 2016-10-13 17:00:26]

The CQ bit was checked by awdf@chromium.org

[awdf, 2016-10-13 17:00:27]

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/2403893002/#ps100001
(title: "Make reply field nullable in .idl files; add test cases for empty and
null")

[commit-bot: I haz the power, 2016-10-13 17:00:46]

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, 2016-10-13 17:13:17]

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

[commit-bot: I haz the power, 2016-10-13 17:13:19]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[awdf, 2016-10-13 17:51:06]

awdf@chromium.org changed reviewers:
+ tkent@chromium.org, tsepez@chromium.org

[awdf, 2016-10-13 17:51:07]

Further to https://codereview.chromium.org/2392343002/ (thanks for your speedy
reviews of that yesterday btw!)

tkent@chromium.org: Please review changes in 
components/test_runner

tsepez@chromium.org: Please review changes in 
content/shell/common/layout_test/layout_test_messages.h (ipc)

[Tom Sepez, 2016-10-13 18:14:43]

lgtm

[tkent, 2016-10-14 00:48:59]

> tkent@chromium.org: Please review changes in 
> components/test_runner

 lgtm

[awdf, 2016-10-14 09:37:09]

The CQ bit was checked by awdf@chromium.org

[commit-bot: I haz the power, 2016-10-14 09:37:19]

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, 2016-10-14 09:43:04]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-10-14 09:45:17]

Description was changed from

==========
Add layout test for reply property of notificationclick event

BUG=599859
==========

to

==========
Add layout test for reply property of notificationclick event

BUG=599859

Committed: https://crrev.com/f8bdbe8a21ba9a9a62755a5aba12770f97234613
Cr-Commit-Position: refs/heads/master@{#425281}
==========

[commit-bot: I haz the power, 2016-10-14 09:45:19]

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