Enable testing NotificationEvent.action

Enable testing NotificationEvent.action

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

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

Part of a sequence of patches:
- https://codereview.chromium.org/1254423011 (blink)
- https://codereview.chromium.org/1263043002 (blink)
- https://codereview.chromium.org/1263043003 (blink)
- https://codereview.chromium.org/1268813007 (blink)
- https://codereview.chromium.org/1267673003 (chromium)
- https://codereview.chromium.org/1262893005 (blink)
- https://codereview.chromium.org/1269923003 (chromium)
- this patch (chromium)
- https://codereview.chromium.org/1264403002 (blink)

BUG=513671
TBR=sky@chromium.org

Committed: https://crrev.com/96d5012f36918b03a447918523acb17548b49fdb
Cr-Commit-Position: refs/heads/master@{#342358}

[johnme, 2015-08-04 15:57:23]

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

[Peter Beverloo, 2015-08-04 21:08:33]

https://codereview.chromium.org/1262893006/diff/1/components/test_runner/test...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1262893006/diff/1/components/test_runner/test...
components/test_runner/test_runner.cc:277: void
SimulateWebNotificationActionClick(const std::string& title,
Can we overload methods here? We shouldn't have two methods for doing
effectively the same thing. If the default bindings don't help you, you can
probably use gin::Arguments* for this.

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

https://codereview.chromium.org/1262893006/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/layout_test_notification_manager.cc:116:
CHECK(action_index < 0) << "Simulating action clicks for page notifications"
Please make sure there's a TODO + a bug for this.

[johnme, 2015-08-05 13:51:27]

Addressed review comments - thanks :)

https://codereview.chromium.org/1262893006/diff/1/components/test_runner/test...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1262893006/diff/1/components/test_runner/test...
components/test_runner/test_runner.cc:277: void
SimulateWebNotificationActionClick(const std::string& title,
On 2015/08/04 21:08:33, Peter Beverloo wrote:
> Can we overload methods here? We shouldn't have two methods for doing
> effectively the same thing. If the default bindings don't help you, you can
> probably use gin::Arguments* for this.

Done, using gin::Arguments*.

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

https://codereview.chromium.org/1262893006/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/layout_test_notification_manager.cc:116:
CHECK(action_index < 0) << "Simulating action clicks for page notifications"
On 2015/08/04 21:08:33, Peter Beverloo wrote:
> Please make sure there's a TODO + a bug for this.

Added a TODO. Plumbing action clicks to page notifications is a core part of
issue 513671 though (and this is how it'll be tested), so no need for an
additional bug.

[johnme, 2015-08-05 13:56:54]

johnme@chromium.org changed reviewers:
+ avi@chromium.org, dpranke@chromium.org, sky@chromium.org

[johnme, 2015-08-05 13:56:55]

avi@chromium.org: Please review changes in content/

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

TBR=sky for no-op change to components/html_viewer/

[Avi (use Gerrit), 2015-08-05 15:07:25]

lgtm

[jochen (gone - plz use gerrit), 2015-08-05 15:09:15]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-08-05 15:09:16]

https://codereview.chromium.org/1262893006/diff/20001/components/test_runner/...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1262893006/diff/20001/components/test_runner/...
components/test_runner/test_runner.cc:1356: args->GetNext(&action_index);
what if the 2nd parameter isn't an integer? shouldn't that throw as well?

[johnme, 2015-08-06 17:56:09]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-08-06 17:56:13]

The CQ bit was unchecked by johnme@chromium.org

[johnme, 2015-08-06 18:25:47]

https://codereview.chromium.org/1262893006/diff/20001/components/test_runner/...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1262893006/diff/20001/components/test_runner/...
components/test_runner/test_runner.cc:1356: args->GetNext(&action_index);
On 2015/08/05 15:09:16, jochen wrote:
> what if the 2nd parameter isn't an integer? shouldn't that throw as well?

Done (oddly, none of the other methods taking gin::Arguments here ever throw;
but I can't see any good reason for that...).

[Dirk Pranke, 2015-08-06 20:43:21]

(I'll defer to jochen here).

[Peter Beverloo, 2015-08-07 00:11:28]

lgtm

I still have a minor personal preference for using a string over an index here,
aiding test legibility, but this won't be nice to implement until
LayoutTestNotificationManager removes the page/persistent duplication. Happy to
go with either.

https://codereview.chromium.org/1262893006/diff/40001/components/test_runner/...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1262893006/diff/40001/components/test_runner/...
components/test_runner/test_runner.cc:1352: !args->PeekNext().IsEmpty() &&
!args->GetNext(&action_index)) {
nit: you could rewrite this statement to be easier to read (negations don't help
- hi TotT!)

==========
if (args->GetNext(&title) &&
    (args->PeekNext().IsEmpty() || args->GetNext(&action_index)))
  runner_->Simultate..

args->ThrowError();
==========

You should consider simply making it required in the call to
SimultateWebNotificationClick, i.e. have this code now, then update all the
tests, then change this away from using gin::Arguments* again.

[johnme, 2015-08-07 07:09:54]

https://codereview.chromium.org/1262893006/diff/40001/components/test_runner/...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1262893006/diff/40001/components/test_runner/...
components/test_runner/test_runner.cc:1352: !args->PeekNext().IsEmpty() &&
!args->GetNext(&action_index)) {
On 2015/08/07 00:11:28, Peter Beverloo wrote:
> nit: you could rewrite this statement to be easier to read (negations don't
help
> - hi TotT!)
> 
> ==========
> if (args->GetNext(&title) &&
>     (args->PeekNext().IsEmpty() || args->GetNext(&action_index)))
>   runner_->Simultate..
> 
> args->ThrowError();
> ==========

Done.

> You should consider simply making it required in the call to
> SimultateWebNotificationClick, i.e. have this code now, then update all the
> tests, then change this away from using gin::Arguments* again.

Acknowledged.

[jochen (gone - plz use gerrit), 2015-08-07 07:12:33]

lgtm

https://codereview.chromium.org/1262893006/diff/60001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_notification_manager.cc
(right):

https://codereview.chromium.org/1262893006/diff/60001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_notification_manager.cc:117:
CHECK(action_index < 0) << "Simulating action clicks for page notifications"
clusterfuzz will love this one... maybe just return?

[johnme, 2015-08-07 14:53:33]

Thanks all!

https://codereview.chromium.org/1262893006/diff/60001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_notification_manager.cc
(right):

https://codereview.chromium.org/1262893006/diff/60001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_notification_manager.cc:117:
CHECK(action_index < 0) << "Simulating action clicks for page notifications"
On 2015/08/07 07:12:33, jochen (traveling - slow) wrote:
> clusterfuzz will love this one... maybe just return?

Done (I removed the CHECK; for now there's actually no harm in firing a plain
NotificationClick; https://codereview.chromium.org/1275743003 will fix this and
add tests).

[johnme, 2015-08-07 14:53:43]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-08-07 14:53:43]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
peter@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1262893006/#ps80001
(title: "Remove check")

[commit-bot: I haz the power, 2015-08-07 14:54:00]

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

[commit-bot: I haz the power, 2015-08-07 15:33:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-07 15:34:25]

Patchset 5 (id:??) landed as
https://crrev.com/96d5012f36918b03a447918523acb17548b49fdb
Cr-Commit-Position: refs/heads/master@{#342358}