Extract the view of control buttons on notification into a separated class

Extract the view of control buttons on notification into a separated class

This patch adds NotificationControlButtonsView, which is the view for the control buttons. It was implemented in ArcNotificationContentView and this patch extracts it into a separated class.

In near feature, NotificationControlButtonsView is shared among non-ARC notification as well.

BUG=717455
TEST=manual test, tests passes

Review-Url: https://codereview.chromium.org/2960333003
Cr-Commit-Position: refs/heads/master@{#483346}
Committed: https://chromium.googlesource.com/chromium/src/+/0962c12a78af3ea8488f9a0525df87651eb48c52

[yoshiki, 2017-06-29 03:35:23]

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

[commit-bot: I haz the power, 2017-06-29 03:35:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yoshiki, 2017-06-29 03:37:44]

Description was changed from

==========
Extract the view of control buttons on notification into the separated class

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

to

==========
Extract the view of control buttons on notification into a separated class

This patch adds NotificationControlButtonsView, which is the view for the
control buttons. It was implemented in ArcNotificationContentView and this patch
extracts it into a separated class.

In near feature, NotificationControlButtonsView is shared among non-ARC
notification as well.

BUG=717455
TEST=manual test, tests passes
==========

[yoshiki, 2017-06-29 03:37:44]

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

[yoshiki, 2017-06-29 03:37:59]

Hanada-san, PTAL. Thanks.

[yhanada, 2017-06-29 04:14:48]

https://codereview.chromium.org/2960333003/diff/1/ui/arc/notification/arc_not...
File ui/arc/notification/arc_notification_content_view.cc (left):

https://codereview.chromium.org/2960333003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_content_view.cc:18: #include
"ui/compositor/layer_animation_observer.h"
LayerAnimationObserver seems to be still used.

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

https://codereview.chromium.org/2960333003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_content_view.cc:17: #include
"ui/base/resource/resource_bundle.h"
It's not related to this change, but this may be able to be removed?

https://codereview.chromium.org/2960333003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_content_view.cc:19: #include
"ui/gfx/animation/linear_animation.h"
remove?

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

https://codereview.chromium.org/2960333003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_content_view.h:129:
message_center::NotificationControlButtonsView* control_buttons_view_ =
too long line?

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

https://codereview.chromium.org/2960333003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_delegate.h:35: bool SettingsClick()
override;
Who calls this method?

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

https://codereview.chromium.org/2960333003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_view.cc:9: #include
"ui/arc/notification/arc_notification_content_view.h"
remove?

https://codereview.chromium.org/2960333003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_view.cc:18: #include
"ui/message_center/views/notification_control_buttons_view.h"
remove?

https://codereview.chromium.org/2960333003/diff/1/ui/message_center/views/not...
File ui/message_center/views/notification_control_buttons_view.cc (right):

https://codereview.chromium.org/2960333003/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_control_buttons_view.cc:47: void
NotificationControlButtonsView::ShowCloseButton(bool show) {
What will happen if the notification is pinned then unpinned then pinned? Can
the order of the buttons change?

https://codereview.chromium.org/2960333003/diff/1/ui/message_center/views/not...
File ui/message_center/views/notification_control_buttons_view.h (right):

https://codereview.chromium.org/2960333003/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_control_buttons_view.h:8: #include
"ui/gfx/geometry/vector2d.h"
Is this needed?

[yoshiki, 2017-06-29 04:22:57]

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

[commit-bot: I haz the power, 2017-06-29 04:23:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yoshiki, 2017-06-29 04:28:21]

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

[commit-bot: I haz the power, 2017-06-29 04:28:40]

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, 2017-06-29 04:45:58]

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

[commit-bot: I haz the power, 2017-06-29 04:45:59]

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

[yoshiki, 2017-06-29 05:30:18]

Patchset #3 (id:40001) has been deleted

[yoshiki, 2017-06-29 05:31:10]

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

[commit-bot: I haz the power, 2017-06-29 05:31:21]

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, 2017-06-29 06:24:53]

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

[commit-bot: I haz the power, 2017-06-29 06:24:53]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[yoshiki, 2017-06-29 06:29:42]

Hanada-san, PTAL. Thanks.

https://codereview.chromium.org/2960333003/diff/1/ui/arc/notification/arc_not...
File ui/arc/notification/arc_notification_content_view.cc (left):

https://codereview.chromium.org/2960333003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_content_view.cc:18: #include
"ui/compositor/layer_animation_observer.h"
On 2017/06/29 04:14:47, yhanada wrote:
> LayerAnimationObserver seems to be still used.

Done.

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

https://codereview.chromium.org/2960333003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_content_view.cc:17: #include
"ui/base/resource/resource_bundle.h"
On 2017/06/29 04:14:47, yhanada wrote:
> It's not related to this change, but this may be able to be removed?

Done.

https://codereview.chromium.org/2960333003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_content_view.cc:19: #include
"ui/gfx/animation/linear_animation.h"
On 2017/06/29 04:14:47, yhanada wrote:
> remove?

Done.

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

https://codereview.chromium.org/2960333003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_content_view.h:129:
message_center::NotificationControlButtonsView* control_buttons_view_ =
On 2017/06/29 04:14:47, yhanada wrote:
> too long line?

No, it has only 73 chars.

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

https://codereview.chromium.org/2960333003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_delegate.h:35: bool SettingsClick()
override;
On 2017/06/29 04:14:47, yhanada wrote:
> Who calls this method?

MessageController calls this method when the settings button is clicked.

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

https://codereview.chromium.org/2960333003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_view.cc:9: #include
"ui/arc/notification/arc_notification_content_view.h"
On 2017/06/29 04:14:47, yhanada wrote:
> remove?

Done.

https://codereview.chromium.org/2960333003/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_view.cc:18: #include
"ui/message_center/views/notification_control_buttons_view.h"
On 2017/06/29 04:14:47, yhanada wrote:
> remove?

Done.

https://codereview.chromium.org/2960333003/diff/1/ui/message_center/views/not...
File ui/message_center/views/notification_control_buttons_view.cc (right):

https://codereview.chromium.org/2960333003/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_control_buttons_view.cc:47: void
NotificationControlButtonsView::ShowCloseButton(bool show) {
On 2017/06/29 04:14:47, yhanada wrote:
> What will happen if the notification is pinned then unpinned then pinned? Can
> the order of the buttons change?

Good catch. I fixed the issue.

https://codereview.chromium.org/2960333003/diff/1/ui/message_center/views/not...
File ui/message_center/views/notification_control_buttons_view.h (right):

https://codereview.chromium.org/2960333003/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_control_buttons_view.h:8: #include
"ui/gfx/geometry/vector2d.h"
On 2017/06/29 04:14:47, yhanada wrote:
> Is this needed?

Removed.

[yhanada, 2017-06-29 06:33:20]

lgtm!

[yoshiki, 2017-06-29 06:35:42]

Thanks! The build failure on win looks unrelated with this issue. Let me take
this into CQ.

[yoshiki, 2017-06-29 06:35:51]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2017-06-29 06:36:02]

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, 2017-06-29 08:02:55]

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

[commit-bot: I haz the power, 2017-06-29 08:02:56]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[yoshiki, 2017-06-29 12:03:01]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2017-06-29 12:03: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, 2017-06-29 13:15:50]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498737781555320,
"parent_rev": "fe6f3f423ad3c90eace89f01cd6be1d4598b81af", "commit_rev":
"0962c12a78af3ea8488f9a0525df87651eb48c52"}

[commit-bot: I haz the power, 2017-06-29 13:16:08]

Description was changed from

==========
Extract the view of control buttons on notification into a separated class

This patch adds NotificationControlButtonsView, which is the view for the
control buttons. It was implemented in ArcNotificationContentView and this patch
extracts it into a separated class.

In near feature, NotificationControlButtonsView is shared among non-ARC
notification as well.

BUG=717455
TEST=manual test, tests passes
==========

to

==========
Extract the view of control buttons on notification into a separated class

This patch adds NotificationControlButtonsView, which is the view for the
control buttons. It was implemented in ArcNotificationContentView and this patch
extracts it into a separated class.

In near feature, NotificationControlButtonsView is shared among non-ARC
notification as well.

BUG=717455
TEST=manual test, tests passes

Review-Url: https://codereview.chromium.org/2960333003
Cr-Commit-Position: refs/heads/master@{#483346}
Committed:
https://chromium.googlesource.com/chromium/src/+/0962c12a78af3ea8488f9a0525df...
==========

[commit-bot: I haz the power, 2017-06-29 13:16:10]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/0962c12a78af3ea8488f9a0525df...