Notification: Implement two-leveled notification.

Notification: Implement two-leveled notification.

The new-style notification should have two states: Collapsed(default) and Expanded.
To implement the feature, this CL does:
* Create NotificationHeaderView which is responsible to keeps app_icon, app_name, expand_button, settings_button, and close_button.
* When the expand_button is clicked, some view's visibility and message_view's line limit is modified to have "expanded" view.

For simplicity, I avoided dynamic creation/deletion of views on expanded state change. The expanded state only changes the existing view's property.
If it hit noticeable performance regression, I'll update the way to change the view. 

BUG=728500, 726242
TEST=manually tested expand/collapse function on following notifications using --enabled-new-style-notification
* A basic notification with one-line message (This doesn't have expand icon.)
* A basic notificatoin with two-line message
* A basic notification with two buttons
* A image notification

Review-Url: https://codereview.chromium.org/2925263003
Cr-Commit-Position: refs/heads/master@{#478587}
Committed: https://chromium.googlesource.com/chromium/src/+/63a16add8253bc6a138329c648ba82a5492f11f0

[fukino, 2017-06-08 22:03:18]

Description was changed from

==========
Notification: Implement two-leveled notification.

BUG=728500,726242
TEST=manually tested expand/collapse function on following notifications using
--enabled-new-style-notification
* A basic notification with one-line message (This doesn't have expand icon.)
* A basic notificatoin with two-line message
* A basic notification with two buttons
* A image notification
==========

to

==========
Notification: Implement two-leveled notification.

The new-style notification should have two states: Collapsed(default) and
Expanded.
To implement the feature, this CL does:
* Create NotificationHeaderView which is responsible to keeps app_icon,
app_name, expand_button, settings_button, and close_button.
* When the expand_button is clicked, some view's visibility and message_view's
line limit is modified to have "expanded" view.

For simplicity, I avoided dynamic creation/deletion of views on expanded state
change. The expanded state only changes the existing view's property.
If it hit noticeable performance regression, I'll update the way to change the
view. 

BUG=728500,726242
TEST=manually tested expand/collapse function on following notifications using
--enabled-new-style-notification
* A basic notification with one-line message (This doesn't have expand icon.)
* A basic notificatoin with two-line message
* A basic notification with two buttons
* A image notification
==========

[fukino, 2017-06-08 22:03:46]

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

[fukino, 2017-06-08 22:03:47]

PTAL. Thanks!

[fukino, 2017-06-08 22:05:48]

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

https://codereview.chromium.org/2925263003/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_view_md.h:89: NotificationHeaderView*
header_row_ = nullptr;
I renamed top_view_, main_view_, and bottom_view to header,content,and actions
to use consistent term with the design mock.

[yoshiki, 2017-06-09 08:48:25]

Thank you for doing this! I did an initial look.

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

https://codereview.chromium.org/2925263003/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_view_md.cc:389: delete image_container_;
How about remote image_container_ from the parent, instead of deleting
image_container_?

I mean, the following is safer:
left_content_->RemoveChildView(image_container_);

https://codereview.chromium.org/2925263003/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_view_md.cc:406:
left_content_->AddChildView(image_container_);
Shouldn't we add the image container into the left container? The width of image
should be same as notification, and we shouldn't subtract the width of the right
content.

https://codereview.chromium.org/2925263003/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_view_md.cc:442: Layout();
Is this Layout necessary?

https://codereview.chromium.org/2925263003/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_view_md.cc:463:
notification.delegate()->ShouldDisplaySettingsButton()) {
nit: braces are not necessary

[fukino, 2017-06-09 09:26:33]

Thank you!

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

https://codereview.chromium.org/2925263003/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_view_md.cc:389: delete image_container_;
On 2017/06/09 08:48:25, yoshiki wrote:
> How about remote image_container_ from the parent, instead of deleting
> image_container_?
> 
> I mean, the following is safer:
> left_content_->RemoveChildView(image_container_);

Done.
Thank you for pointing it out.

https://codereview.chromium.org/2925263003/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_view_md.cc:406:
left_content_->AddChildView(image_container_);
On 2017/06/09 08:48:25, yoshiki wrote:
> Shouldn't we add the image container into the left container? The width of
image
> should be same as notification, and we shouldn't subtract the width of the
right
> content.

The big image is shown only when the notification is expanded.
When the notification is expanded, the right_content_ gets invisible so the
image use whole width of the notification.

https://codereview.chromium.org/2925263003/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_view_md.cc:442: Layout();
On 2017/06/09 08:48:25, yoshiki wrote:
> Is this Layout necessary?

I blindly copied it from notification_view.
https://codesearch.chromium.org/chromium/src/ui/message_center/views/notifica...

So far I didn't see any issues without this Layout(), but I'm confident I
considered edge cases.
I left TODO comment to investigate it more.

https://codereview.chromium.org/2925263003/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_view_md.cc:463:
notification.delegate()->ShouldDisplaySettingsButton()) {
On 2017/06/09 08:48:25, yoshiki wrote:
> nit: braces are not necessary

Done.

[fukino, 2017-06-12 09:07:38]

yoshiki@, could you take another look?

To make the behavior more consistent with Android notifications, I moved
|image_container_| out of left container and keep notification icons visible
when it is expanded.

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

https://codereview.chromium.org/2925263003/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_view_md.cc:406:
left_content_->AddChildView(image_container_);
On 2017/06/09 09:26:32, fukino wrote:
> On 2017/06/09 08:48:25, yoshiki wrote:
> > Shouldn't we add the image container into the left container? The width of
> image
> > should be same as notification, and we shouldn't subtract the width of the
> right
> > content.
> 
> The big image is shown only when the notification is expanded.
> When the notification is expanded, the right_content_ gets invisible so the
> image use whole width of the notification.

According to the Android current behavior, the notification icon will not get
invisible when the notification is expanded.
To align with it, I moved |image_container_| out of |content_row_|.

[yoshiki, 2017-06-12 09:33:17]

lgtm. Thanks.

[fukino, 2017-06-12 09:36:08]

On 2017/06/12 09:33:17, yoshiki wrote:
> lgtm. Thanks.

Thank you for the review!

[fukino, 2017-06-12 09:36:12]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2017-06-12 09:36:28]

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-12 10:56:19]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497260172795820,
"parent_rev": "2b6cc8a0982c39253fecc01d9a35dbc7a4f07480", "commit_rev":
"63a16add8253bc6a138329c648ba82a5492f11f0"}

[commit-bot: I haz the power, 2017-06-12 10:56:33]

Description was changed from

==========
Notification: Implement two-leveled notification.

The new-style notification should have two states: Collapsed(default) and
Expanded.
To implement the feature, this CL does:
* Create NotificationHeaderView which is responsible to keeps app_icon,
app_name, expand_button, settings_button, and close_button.
* When the expand_button is clicked, some view's visibility and message_view's
line limit is modified to have "expanded" view.

For simplicity, I avoided dynamic creation/deletion of views on expanded state
change. The expanded state only changes the existing view's property.
If it hit noticeable performance regression, I'll update the way to change the
view. 

BUG=728500,726242
TEST=manually tested expand/collapse function on following notifications using
--enabled-new-style-notification
* A basic notification with one-line message (This doesn't have expand icon.)
* A basic notificatoin with two-line message
* A basic notification with two buttons
* A image notification
==========

to

==========
Notification: Implement two-leveled notification.

The new-style notification should have two states: Collapsed(default) and
Expanded.
To implement the feature, this CL does:
* Create NotificationHeaderView which is responsible to keeps app_icon,
app_name, expand_button, settings_button, and close_button.
* When the expand_button is clicked, some view's visibility and message_view's
line limit is modified to have "expanded" view.

For simplicity, I avoided dynamic creation/deletion of views on expanded state
change. The expanded state only changes the existing view's property.
If it hit noticeable performance regression, I'll update the way to change the
view. 

BUG=728500,726242
TEST=manually tested expand/collapse function on following notifications using
--enabled-new-style-notification
* A basic notification with one-line message (This doesn't have expand icon.)
* A basic notificatoin with two-line message
* A basic notification with two buttons
* A image notification

Review-Url: https://codereview.chromium.org/2925263003
Cr-Commit-Position: refs/heads/master@{#478587}
Committed:
https://chromium.googlesource.com/chromium/src/+/63a16add8253bc6a138329c648ba...
==========

[commit-bot: I haz the power, 2017-06-12 10:56:34]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/63a16add8253bc6a138329c648ba...