Transfer responsibility for providing a close button for a notification to each implementation of M…

Transfer responsibility for providing a close button for a notification to each implementation of MessageView.

Because content of |CustomNotificationView| covers a close button
that is owned by |MessageView|, |CustomNotificationView| provides
their own close button. This causes inconsistent behavior of the
close buttons between normal notifications and custom notifications.

- Transfer the close button which is owned by |MessageView| to
  |NotificationView|
- Introduce |CustomNotificationContentViewDelegate| class for
  delegating handling of a close button to a content view hosted by
  |CustomNotificationView|

BUG=661105
TEST=Manual test. The keyboard focus is moved to a close button of
     a custom notification when the close button of the next
     notification is focused and the next notification is removed.

Committed: https://crrev.com/a53dbe07cd6f4abd9ce526cc07c118518654a440
Cr-Commit-Position: refs/heads/master@{#436827}

[yhanada, 2016-12-01 05:44:53]

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

[commit-bot: I haz the power, 2016-12-01 05:45:04]

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-12-01 06:36:26]

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

[commit-bot: I haz the power, 2016-12-01 06:36:27]

Dry run: This issue passed the CQ dry run.

[yhanada, 2016-12-01 06:55:33]

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

[yhanada, 2016-12-01 06:55:33]

Hi yoshiki@, PTAL.

[yoshiki, 2016-12-02 12:15:04]

Description was changed from

==========
Transfer responsibility for providing a close button for a notification to each
implementation of MessageView.

Because content of |CustomNotificationView| covers a close button
that is owned by |MessageView|, |CustomNotificationView| provides
their own close button. This causes inconsistent behavior of the
close buttons between normal notifications and custom notifications.

- Transfer the close button which is owned by |MessageView| to
  |NotificationView|
- Introduce |CustomNotificationContentViewDelegate| class for
  delegating handling of a close button to a content view hosted by
  |CustomNotificationView|

BUG=661105
TEST=Manual test. The keyboard focus is moved to a close button of
     a custom notification when the close button of the next
     notification is focused and the next notification is removed.
==========

to

==========
Transfer responsibility for providing a close button for a notification to each
implementation of MessageView.

Because content of |CustomNotificationView| covers a close button
that is owned by |MessageView|, |CustomNotificationView| provides
their own close button. This causes inconsistent behavior of the
close buttons between normal notifications and custom notifications.

- Transfer the close button which is owned by |MessageView| to
  |NotificationView|
- Introduce |CustomNotificationContentViewDelegate| class for
  delegating handling of a close button to a content view hosted by
  |CustomNotificationView|

BUG=661105
TEST=Manual test. The keyboard focus is moved to a close button of
     a custom notification when the close button of the next
     notification is focused and the next notification is removed.
==========

[yoshiki, 2016-12-02 12:15:04]

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

[yoshiki, 2016-12-02 12:15:48]

+xiyuan may have other comments.

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

https://codereview.chromium.org/2547433002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_item.cc:30: std::tuple<
Instead of using a tuple, how about creating new class which has 2 properties
for views::View and CustomNotificationContentViewDelegate?

Or the following might also be good.
1. Move the code creating ArcCustomNotificationView instance (l.34) to the
constructor.
2. Rename CreateCustomContent() method to GetCustomContentView()
3. Add GetCustomContentViewDelegate() method which returns a delegate instance
to NotificationDelegate class.

IMO, using a tuple here looks somewhat hard to read for me.

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

https://codereview.chromium.org/2547433002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:171:
ArcCustomNotificationView* const owner_;
Please add DISALLOW_COPY_AND_ASSIGN

[xiyuan, 2016-12-02 17:45:53]

Overall approach looks good.

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

https://codereview.chromium.org/2547433002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_item.cc:30: std::tuple<
On 2016/12/02 12:15:48, yoshiki wrote:
> Instead of using a tuple, how about creating new class which has 2 properties
> for views::View and CustomNotificationContentViewDelegate?
> 

+1 to this, e.g. making struct CustomContent instead of a tuple to hold the two.

> Or the following might also be good.
> 1. Move the code creating ArcCustomNotificationView instance (l.34) to the
> constructor.
> 2. Rename CreateCustomContent() method to GetCustomContentView()
> 3. Add GetCustomContentViewDelegate() method which returns a delegate instance
> to NotificationDelegate class.
> 

NotificationDelegate is expected to create a custom content view that is going
to be owned  by CustomNotificationView later on. It would be hard to split this
into two functions and correctly associate the CustomContentViewDelegat with the
view.

[yhanada, 2016-12-06 02:48:08]

Thank you all for reviewing! I created a new struct |CustomContent| for holding
the view and the delegate. Please take another look.

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

https://codereview.chromium.org/2547433002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_item.cc:30: std::tuple<
Created |CustomContent| struct for holding the two. Done.

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

https://codereview.chromium.org/2547433002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:171:
ArcCustomNotificationView* const owner_;
On 2016/12/02 12:15:48, yoshiki wrote:
> Please add DISALLOW_COPY_AND_ASSIGN

Done.

[yoshiki, 2016-12-06 05:28:28]

https://codereview.chromium.org/2547433002/diff/40001/ui/arc/notification/arc...
File ui/arc/notification/arc_custom_notification_view.cc (right):

https://codereview.chromium.org/2547433002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:207:
ArcCustomNotificationView::CreateContentViewDelegate() {
Shouldn't we need to add 4-space indent?

https://codereview.chromium.org/2547433002/diff/40001/ui/arc/notification/arc...
File ui/arc/notification/arc_custom_notification_view.h (right):

https://codereview.chromium.org/2547433002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.h:41:
CreateContentViewDelegate();
Shouldn't we need to add 4-space indent?

https://codereview.chromium.org/2547433002/diff/40001/ui/message_center/notif...
File ui/message_center/notification_delegate.h (right):

https://codereview.chromium.org/2547433002/diff/40001/ui/message_center/notif...
ui/message_center/notification_delegate.h:10: #include <tuple>
Please remove this change.

https://codereview.chromium.org/2547433002/diff/40001/ui/message_center/views...
File ui/message_center/views/custom_notification_content_view_delegate.h
(right):

https://codereview.chromium.org/2547433002/diff/40001/ui/message_center/views...
ui/message_center/views/custom_notification_content_view_delegate.h:15: }
Please add "// namespace views"

https://codereview.chromium.org/2547433002/diff/40001/ui/message_center/views...
ui/message_center/views/custom_notification_content_view_delegate.h:36:
std::unique_ptr<views::View> view_;
Remove the underscode at the end since this is public.

https://codereview.chromium.org/2547433002/diff/40001/ui/message_center/views...
File ui/message_center/views/custom_notification_view.h (right):

https://codereview.chromium.org/2547433002/diff/40001/ui/message_center/views...
ui/message_center/views/custom_notification_view.h:38: contents_view_delegate_ =
nullptr;
Assigning nullptr is not necessary here.

https://codereview.chromium.org/2547433002/diff/40001/ui/message_center/views...
File ui/message_center/views/notification_view.h (right):

https://codereview.chromium.org/2547433002/diff/40001/ui/message_center/views...
ui/message_center/views/notification_view.h:52: bool IsCloseButtonFocused()
const override;
Please move this line to a separated block since this is overridden from
views::ButtonListener

[yhanada, 2016-12-06 06:41:26]

https://codereview.chromium.org/2547433002/diff/40001/ui/arc/notification/arc...
File ui/arc/notification/arc_custom_notification_view.cc (right):

https://codereview.chromium.org/2547433002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:207:
ArcCustomNotificationView::CreateContentViewDelegate() {
On 2016/12/06 05:28:27, yoshiki wrote:
> Shouldn't we need to add 4-space indent?

I'm formatting this CL by 'git cl format' command.

https://codereview.chromium.org/2547433002/diff/40001/ui/message_center/notif...
File ui/message_center/notification_delegate.h (right):

https://codereview.chromium.org/2547433002/diff/40001/ui/message_center/notif...
ui/message_center/notification_delegate.h:10: #include <tuple>
On 2016/12/06 05:28:27, yoshiki wrote:
> Please remove this change.

Done.

https://codereview.chromium.org/2547433002/diff/40001/ui/message_center/views...
File ui/message_center/views/custom_notification_content_view_delegate.h
(right):

https://codereview.chromium.org/2547433002/diff/40001/ui/message_center/views...
ui/message_center/views/custom_notification_content_view_delegate.h:15: }
On 2016/12/06 05:28:27, yoshiki wrote:
> Please add "// namespace views"

Done.

https://codereview.chromium.org/2547433002/diff/40001/ui/message_center/views...
ui/message_center/views/custom_notification_content_view_delegate.h:36:
std::unique_ptr<views::View> view_;
On 2016/12/06 05:28:27, yoshiki wrote:
> Remove the underscode at the end since this is public.

Done.

https://codereview.chromium.org/2547433002/diff/40001/ui/message_center/views...
File ui/message_center/views/custom_notification_view.h (right):

https://codereview.chromium.org/2547433002/diff/40001/ui/message_center/views...
ui/message_center/views/custom_notification_view.h:38: contents_view_delegate_ =
nullptr;
On 2016/12/06 05:28:27, yoshiki wrote:
> Assigning nullptr is not necessary here.

Done.

https://codereview.chromium.org/2547433002/diff/40001/ui/message_center/views...
File ui/message_center/views/notification_view.h (right):

https://codereview.chromium.org/2547433002/diff/40001/ui/message_center/views...
ui/message_center/views/notification_view.h:52: bool IsCloseButtonFocused()
const override;
On 2016/12/06 05:28:27, yoshiki wrote:
> Please move this line to a separated block since this is overridden from
> views::ButtonListener

These methods are actually from MessageView.

[yoshiki, 2016-12-06 08:31:54]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2016-12-06 08:31:55]

lgtm

>?>984234444444444444444444444445123`111111111111111111111111111111111111111111111111111111111111432qwerrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrewwwwwwwwwwwwewqqqqqqqqqqqqqqqqqqqqwqqqeeeeeeeeeeeeeadfsqqqqqqqqqqqqqqqqqqqqqqqqqqqwerrtrte

[commit-bot: I haz the power, 2016-12-06 08:32:19]

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

[yoshiki, 2016-12-06 08:33:53]

The CQ bit was unchecked by yoshiki@chromium.org

[yhanada, 2016-12-06 08:34:22]

The CQ bit was unchecked by yhanada@chromium.org

[yoshiki, 2016-12-06 08:34:48]

Sorry, I was cleaning the keyboard. The patch lgtm.

[yoshiki, 2016-12-06 08:34:50]

Sorry, I was cleaning the keyboard. The patch lgtm.

[xiyuan, 2016-12-06 17:11:16]

lgtm

https://codereview.chromium.org/2547433002/diff/60001/ui/message_center/views...
File ui/message_center/views/custom_notification_view_unittest.cc (right):

https://codereview.chromium.org/2547433002/diff/60001/ui/message_center/views...
ui/message_center/views/custom_notification_view_unittest.cc:84:
base::WrapUnique(new TestContentViewDelegate));
nit: WrapUnique -> MakeUnique

[yhanada, 2016-12-07 00:23:25]

Thank you for reviewing!

https://codereview.chromium.org/2547433002/diff/60001/ui/message_center/views...
File ui/message_center/views/custom_notification_view_unittest.cc (right):

https://codereview.chromium.org/2547433002/diff/60001/ui/message_center/views...
ui/message_center/views/custom_notification_view_unittest.cc:84:
base::WrapUnique(new TestContentViewDelegate));
On 2016/12/06 17:11:16, xiyuan wrote:
> nit: WrapUnique -> MakeUnique

Done.

[yhanada, 2016-12-07 00:23:30]

The CQ bit was checked by yhanada@chromium.org

[yhanada, 2016-12-07 00:23:30]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
yoshiki@chromium.org
Link to the patchset: https://codereview.chromium.org/2547433002/#ps100001
(title: "WrapUnique -> MakeUnique")

[commit-bot: I haz the power, 2016-12-07 00:24:05]

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-12-07 01:39:50]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481070210097730,
"parent_rev": "b0fb74bfe08cabef780dbd7579e74a5230870032", "commit_rev":
"0d87dba97c0d6d7d7440a171f4856a97d7a2db62"}

[commit-bot: I haz the power, 2016-12-07 01:40:14]

Description was changed from

==========
Transfer responsibility for providing a close button for a notification to each
implementation of MessageView.

Because content of |CustomNotificationView| covers a close button
that is owned by |MessageView|, |CustomNotificationView| provides
their own close button. This causes inconsistent behavior of the
close buttons between normal notifications and custom notifications.

- Transfer the close button which is owned by |MessageView| to
  |NotificationView|
- Introduce |CustomNotificationContentViewDelegate| class for
  delegating handling of a close button to a content view hosted by
  |CustomNotificationView|

BUG=661105
TEST=Manual test. The keyboard focus is moved to a close button of
     a custom notification when the close button of the next
     notification is focused and the next notification is removed.
==========

to

==========
Transfer responsibility for providing a close button for a notification to each
implementation of MessageView.

Because content of |CustomNotificationView| covers a close button
that is owned by |MessageView|, |CustomNotificationView| provides
their own close button. This causes inconsistent behavior of the
close buttons between normal notifications and custom notifications.

- Transfer the close button which is owned by |MessageView| to
  |NotificationView|
- Introduce |CustomNotificationContentViewDelegate| class for
  delegating handling of a close button to a content view hosted by
  |CustomNotificationView|

BUG=661105
TEST=Manual test. The keyboard focus is moved to a close button of
     a custom notification when the close button of the next
     notification is focused and the next notification is removed.
==========

[commit-bot: I haz the power, 2016-12-07 01:40:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-07 01:42:41]

Description was changed from

==========
Transfer responsibility for providing a close button for a notification to each
implementation of MessageView.

Because content of |CustomNotificationView| covers a close button
that is owned by |MessageView|, |CustomNotificationView| provides
their own close button. This causes inconsistent behavior of the
close buttons between normal notifications and custom notifications.

- Transfer the close button which is owned by |MessageView| to
  |NotificationView|
- Introduce |CustomNotificationContentViewDelegate| class for
  delegating handling of a close button to a content view hosted by
  |CustomNotificationView|

BUG=661105
TEST=Manual test. The keyboard focus is moved to a close button of
     a custom notification when the close button of the next
     notification is focused and the next notification is removed.
==========

to

==========
Transfer responsibility for providing a close button for a notification to each
implementation of MessageView.

Because content of |CustomNotificationView| covers a close button
that is owned by |MessageView|, |CustomNotificationView| provides
their own close button. This causes inconsistent behavior of the
close buttons between normal notifications and custom notifications.

- Transfer the close button which is owned by |MessageView| to
  |NotificationView|
- Introduce |CustomNotificationContentViewDelegate| class for
  delegating handling of a close button to a content view hosted by
  |CustomNotificationView|

BUG=661105
TEST=Manual test. The keyboard focus is moved to a close button of
     a custom notification when the close button of the next
     notification is focused and the next notification is removed.

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

[commit-bot: I haz the power, 2016-12-07 01:42:43]

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