Support buttons on ARC Notifications

Support buttons on ARC Notifications

This patch adds button support on ARC notification: showing buttons and handle click events.

This CL depends on the Android side patch (http://ag/845794).

BUG=560682
TEST=manually tested

Committed: https://crrev.com/4b77bc2aeb88aa05f56b3de2d1effd7d0918e395
Cr-Commit-Position: refs/heads/master@{#371490}

[yoshiki, 2016-01-19 02:53:42]

Description was changed from

==========
wip

BUG=
==========

to

==========
Support progress-bar and buttons on ARC Notifications


BUG=560682
==========

[yoshiki, 2016-01-19 02:53:42]

yoshiki@chromium.org changed reviewers:
+ hidehiko@chromium.org

[yoshiki, 2016-01-19 02:54:30]

Description was changed from

==========
Support progress-bar and buttons on ARC Notifications


BUG=560682
==========

to

==========
Support buttons on ARC Notifications


BUG=560682
==========

[yoshiki, 2016-01-19 02:57:09]

Description was changed from

==========
Support buttons on ARC Notifications


BUG=560682
==========

to

==========
Support buttons on ARC Notifications

This patch adds button support on ARC notification: showing buttons and handle
click events.

BUG=560682
TEST=manually tested
==========

[yoshiki, 2016-01-19 03:10:43]

Description was changed from

==========
Support buttons on ARC Notifications

This patch adds button support on ARC notification: showing buttons and handle
click events.

BUG=560682
TEST=manually tested
==========

to

==========
Support buttons on ARC Notifications

This patch adds button support on ARC notification: showing buttons and handle
click events.

This patch depends on the Android side patch: http://ag/845794

BUG=560682
TEST=manually tested
==========

[yoshiki, 2016-01-19 03:10:52]

Description was changed from

==========
Support buttons on ARC Notifications

This patch adds button support on ARC notification: showing buttons and handle
click events.

This patch depends on the Android side patch: http://ag/845794

BUG=560682
TEST=manually tested
==========

to

==========
Support buttons on ARC Notifications

This patch adds button support on ARC notification: showing buttons and handle
click events.

This CL depends on the Android side patch: http://ag/845794

BUG=560682
TEST=manually tested
==========

[yoshiki, 2016-01-19 03:11:30]

Hidehiko-san, PTAL. Thanks.

[yoshiki, 2016-01-21 12:16:08]

Description was changed from

==========
Support buttons on ARC Notifications

This patch adds button support on ARC notification: showing buttons and handle
click events.

This CL depends on the Android side patch: http://ag/845794

BUG=560682
TEST=manually tested
==========

to

==========
Support buttons on ARC Notifications

This patch adds button support on ARC notification: showing buttons and handle
click events.

This CL depends on the IPC message patch (http://crrev.com/1615813002) and the
Android side patch (http://ag/845794).

BUG=560682
TEST=manually tested
==========

[yoshiki, 2016-01-22 02:09:20]

Hidehiko-sa, ping.

[hidehiko, 2016-01-22 07:47:04]

LGTM with nitpick.
(Sorry for delay. My comments stayed as draft... :-(

https://codereview.chromium.org/1603753003/diff/1/ui/arc/notification/arc_not...
File ui/arc/notification/arc_notification_item.h (right):

https://codereview.chromium.org/1603753003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_item.h:38: void ButtonClick(int
button_index);
nit: probably ClickButton() sounds more verb phrase?

https://codereview.chromium.org/1603753003/diff/1/ui/arc/notification/arc_not...
File ui/arc/notification/arc_notification_manager.cc (right):

https://codereview.chromium.org/1603753003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_manager.cc:98: << key << "), but it is
gone.";
nit: maybe "it has gone"?

https://codereview.chromium.org/1603753003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_manager.cc:104: case 0:  command =
ARC_NOTIFICATION_EVENT_BUTTON1_CLICKED;  break;
Let's follow the style guide:
https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements

[yoshiki, 2016-01-25 15:45:50]

https://codereview.chromium.org/1603753003/diff/1/ui/arc/notification/arc_not...
File ui/arc/notification/arc_notification_item.h (right):

https://codereview.chromium.org/1603753003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_item.h:38: void ButtonClick(int
button_index);
On 2016/01/22 07:47:03, hidehiko wrote:
> nit: probably ClickButton() sounds more verb phrase?

Let me keep it ButtonClick, since NotificationDelegate has the method named
ButtonClick and using same name is better.

https://codereview.chromium.org/1603753003/diff/1/ui/arc/notification/arc_not...
File ui/arc/notification/arc_notification_manager.cc (right):

https://codereview.chromium.org/1603753003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_manager.cc:98: << key << "), but it is
gone.";
On 2016/01/22 07:47:03, hidehiko wrote:
> nit: maybe "it has gone"?

Done.

https://codereview.chromium.org/1603753003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_manager.cc:104: case 0:  command =
ARC_NOTIFICATION_EVENT_BUTTON1_CLICKED;  break;
On 2016/01/22 07:47:03, hidehiko wrote:
> Let's follow the style guide:
> https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements

Done.

[yoshiki, 2016-01-25 15:46:14]

yoshiki@chromium.org changed reviewers:
+ dcheng@chromium.org

[yoshiki, 2016-01-25 15:46:34]

Description was changed from

==========
Support buttons on ARC Notifications

This patch adds button support on ARC notification: showing buttons and handle
click events.

This CL depends on the IPC message patch (http://crrev.com/1615813002) and the
Android side patch (http://ag/845794).

BUG=560682
TEST=manually tested
==========

to

==========
Support buttons on ARC Notifications

This patch adds button support on ARC notification: showing buttons and handle
click events.

This CL depends on the Android side patch (http://ag/845794).

BUG=560682
TEST=manually tested
==========

[yoshiki, 2016-01-25 15:47:14]

@dcheng, could you take a look at the *.mojom change? Thanks

[hidehiko, 2016-01-26 08:08:40]

On 2016/01/25 15:47:14, yoshiki wrote:
> @dcheng, could you take a look at the *.mojom change? Thanks

The trybot failure looks real ones. Please fix them.

[dcheng, 2016-01-26 08:42:08]

mojom lgtm

https://codereview.chromium.org/1603753003/diff/20001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_manager.cc (right):

https://codereview.chromium.org/1603753003/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_manager.cc:59: << "), but it has already
gone.";
Nit: "it is" here and elsewhere is probably more idiomatic english.

[yoshiki, 2016-01-26 11:01:55]

The build failures are caused by the recent mojo roll. I updated the patch and
the problems are fixed.

[hidehiko, 2016-01-26 11:03:51]

lgtm

[yoshiki, 2016-01-26 11:14:47]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2016-01-26 11:14:47]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1603753003/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2016-01-26 11:15:00]

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

[commit-bot: I haz the power, 2016-01-26 11:19:34]

Description was changed from

==========
Support buttons on ARC Notifications

This patch adds button support on ARC notification: showing buttons and handle
click events.

This CL depends on the Android side patch (http://ag/845794).

BUG=560682
TEST=manually tested
==========

to

==========
Support buttons on ARC Notifications

This patch adds button support on ARC notification: showing buttons and handle
click events.

This CL depends on the Android side patch (http://ag/845794).

BUG=560682
TEST=manually tested
==========

[commit-bot: I haz the power, 2016-01-26 11:19:35]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-26 11:20:55]

Description was changed from

==========
Support buttons on ARC Notifications

This patch adds button support on ARC notification: showing buttons and handle
click events.

This CL depends on the Android side patch (http://ag/845794).

BUG=560682
TEST=manually tested
==========

to

==========
Support buttons on ARC Notifications

This patch adds button support on ARC notification: showing buttons and handle
click events.

This CL depends on the Android side patch (http://ag/845794).

BUG=560682
TEST=manually tested

Committed: https://crrev.com/4b77bc2aeb88aa05f56b3de2d1effd7d0918e395
Cr-Commit-Position: refs/heads/master@{#371490}
==========

[commit-bot: I haz the power, 2016-01-26 11:20:56]

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