Implement Non-Closable Notification

Implement Non-Closable Notification

This patch adds pinned notifications, which can't be removed by user from the message center. This feature is only for ChromeOS, since the message center doesn't exist on the other platforms.

All toasts must be closable. But if the closing toast has non-closable attribute, it'll be kept in the message center after closing.

BUG=565005
R=dewittj@chromium.org

Committed: https://crrev.com/35c9a3b689e257570a0ba08af1c6e0dc18ac3cc1
Cr-Commit-Position: refs/heads/master@{#377517}

[yoshiki, 2016-01-29 01:32:23]

Description was changed from

==========
wip

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

to

==========
wip: Implement Nonclosable Notification

BUG=565005
==========

[yoshiki, 2016-02-02 03:05:54]

Patchset #5 (id:80001) has been deleted

[yoshiki, 2016-02-02 03:06:35]

Patchset #3 (id:40001) has been deleted

[yoshiki, 2016-02-02 03:06:55]

Patchset #1 (id:1) has been deleted

[yoshiki, 2016-02-02 03:07:11]

Patchset #2 (id:60001) has been deleted

[yoshiki, 2016-02-02 03:07:23]

Patchset #1 (id:20001) has been deleted

[yoshiki, 2016-02-02 03:25:52]

Description was changed from

==========
wip: Implement Nonclosable Notification

BUG=565005
==========

to

==========
Implement Non-Closable Notification

This patch adds sticky notifications, which can't be removed by user from the
message center. This is only for ChromeOS, since the message center doesn't
exist on the other platforms. 

All toasts must be closable. But if the closing toast has non-closable
attribute, it'll be kept in the message center after closing. 

BUG=565005
==========

[yoshiki, 2016-02-02 03:25:52]

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

[yoshiki, 2016-02-02 03:27:15]

Patchset #1 (id:100001) has been deleted

[yoshiki, 2016-02-02 03:28:36]

Description was changed from

==========
Implement Non-Closable Notification

This patch adds sticky notifications, which can't be removed by user from the
message center. This is only for ChromeOS, since the message center doesn't
exist on the other platforms. 

All toasts must be closable. But if the closing toast has non-closable
attribute, it'll be kept in the message center after closing. 

BUG=565005
==========

to

==========
Implement Non-Closable Notification

This patch adds sticky notifications, which can't be removed by user from the
message center. This is only for ChromeOS, since the message center doesn't
exist on the other platforms. 

All toasts must be closable. But if the closing toast has non-closable
attribute, it'll be kept in the message center after closing. 

BUG=565005
R=dewittj@chromium.org
==========

[yoshiki, 2016-02-02 03:30:22]

Patchset #1 (id:120001) has been deleted

[yoshiki, 2016-02-02 03:30:39]

Justin, PTAL. Thanks.

[dewittj, 2016-02-03 19:31:38]

This is a large patch, so my comments may be partial.  I'm still trying to fully
wrap my head around it.  In the mean time, here are some comments.

Do you plan to integrate with the extension or web APIs ever or is this just an
internal C++-only feature?

https://codereview.chromium.org/1645843003/diff/140001/ui/message_center/mess...
File ui/message_center/message_center.h (right):

https://codereview.chromium.org/1645843003/diff/140001/ui/message_center/mess...
ui/message_center/message_center.h:103: // Removes an existing notification.
All the remove methods should have their own comments now, since it's gotten so
complicated.  It's unclear which will be removed and when.   Should we simplify
this API by passing a RemoveParams type object that specifies the type of things
to remove?

https://codereview.chromium.org/1645843003/diff/140001/ui/message_center/view...
File ui/message_center/views/message_center_button_bar.h (right):

https://codereview.chromium.org/1645843003/diff/140001/ui/message_center/view...
ui/message_center/views/message_center_button_bar.h:53:
FRIEND_TEST_ALL_PREFIXES(MessageCenterViewTest, CloseButton);
Why do you need this if you have the ForTest() method?  If it's for the
ButtonPressed method, then possibly you should just upcast it to ButtonListener
and call the public method from that interface.

https://codereview.chromium.org/1645843003/diff/140001/ui/message_center/view...
File ui/message_center/views/message_popup_collection.cc (right):

https://codereview.chromium.org/1645843003/diff/140001/ui/message_center/view...
ui/message_center/views/message_popup_collection.cc:98: if
(notification->closable())
Should there be an else clause, something like
"MarkSinglePopupAsShown(notification_id)" so it goes to the NC

https://codereview.chromium.org/1645843003/diff/140001/ui/views/controls/slid...
File ui/views/controls/slide_out_view.h (right):

https://codereview.chromium.org/1645843003/diff/140001/ui/views/controls/slid...
ui/views/controls/slide_out_view.h:24: bool closable() { return closable_; }
It seems like this is not the appropriate place for this flag.

Maybe it should be slide_out_enabled() instead, so it's more general?

[yoshiki, 2016-02-05 23:50:30]

Patchset #2 (id:160001) has been deleted

[yoshiki, 2016-02-05 23:51:04]

Patchset #2 (id:180001) has been deleted

[yoshiki, 2016-02-05 23:52:02]

Patchset #2 (id:200001) has been deleted

[yoshiki, 2016-02-06 07:09:13]

Patchset #4 (id:260001) has been deleted

[yoshiki, 2016-02-06 07:09:27]

Patchset #2 (id:220001) has been deleted

[yoshiki, 2016-02-06 07:09:36]

Patchset #2 (id:240001) has been deleted

[yoshiki, 2016-02-07 17:42:44]

Patchset #2 (id:280001) has been deleted

[yoshiki, 2016-02-07 17:49:31]

On 2016/02/03 19:31:38, dewittj wrote:
> This is a large patch, so my comments may be partial.  I'm still trying to
fully
> wrap my head around it.  In the mean time, here are some comments.

Thanks, thank you for taking a time!

> Do you plan to integrate with the extension or web APIs ever or is this just
an
> internal C++-only feature?

Currently, I don't have a plan to expose this to extension/web API. If someone
want to do the work, I'll intend to help them.

[yoshiki, 2016-02-07 17:49:41]

https://codereview.chromium.org/1645843003/diff/140001/ui/message_center/mess...
File ui/message_center/message_center.h (right):

https://codereview.chromium.org/1645843003/diff/140001/ui/message_center/mess...
ui/message_center/message_center.h:103: // Removes an existing notification.
On 2016/02/03 19:31:37, dewittj wrote:
> All the remove methods should have their own comments now, since it's gotten
so
> complicated.  It's unclear which will be removed and when.   Should we
simplify
> this API by passing a RemoveParams type object that specifies the type of
things
> to remove?

I think RemoveNotification can be kept as an independent method for convenience.
So I combined the RemoveAll*Notifications methods into one.

https://codereview.chromium.org/1645843003/diff/140001/ui/message_center/view...
File ui/message_center/views/message_center_button_bar.h (right):

https://codereview.chromium.org/1645843003/diff/140001/ui/message_center/view...
ui/message_center/views/message_center_button_bar.h:53:
FRIEND_TEST_ALL_PREFIXES(MessageCenterViewTest, CloseButton);
On 2016/02/03 19:31:37, dewittj wrote:
> Why do you need this if you have the ForTest() method?  If it's for the
> ButtonPressed method, then possibly you should just upcast it to
ButtonListener
> and call the public method from that interface.

Done.

https://codereview.chromium.org/1645843003/diff/140001/ui/message_center/view...
File ui/message_center/views/message_popup_collection.cc (right):

https://codereview.chromium.org/1645843003/diff/140001/ui/message_center/view...
ui/message_center/views/message_popup_collection.cc:98: if
(notification->closable())
On 2016/02/03 19:31:37, dewittj wrote:
> Should there be an else clause, something like
> "MarkSinglePopupAsShown(notification_id)" so it goes to the NC

Good point. Done.

https://codereview.chromium.org/1645843003/diff/140001/ui/views/controls/slid...
File ui/views/controls/slide_out_view.h (right):

https://codereview.chromium.org/1645843003/diff/140001/ui/views/controls/slid...
ui/views/controls/slide_out_view.h:24: bool closable() { return closable_; }
On 2016/02/03 19:31:37, dewittj wrote:
> It seems like this is not the appropriate place for this flag.
> 
> Maybe it should be slide_out_enabled() instead, so it's more general?

Done.

[dewittj, 2016-02-08 18:02:35]

https://codereview.chromium.org/1645843003/diff/300001/ash/accelerators/accel...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/1645843003/diff/300001/ash/accelerators/accel...
ash/accelerators/accelerator_controller_unittest.cc:1523: false /* by_usr */,
message_center::MessageCenter::RemoveType::ALL);
nit: by_user

https://codereview.chromium.org/1645843003/diff/300001/ui/message_center/mess...
File ui/message_center/message_center.h (right):

https://codereview.chromium.org/1645843003/diff/300001/ui/message_center/mess...
ui/message_center/message_center.h:54: // Remove all notifications (including
invisible ones and unclosable ones).
nit: remove the parenthetical phrase - I think |all| is clear enough.

https://codereview.chromium.org/1645843003/diff/300001/ui/message_center/mess...
ui/message_center/message_center.h:57: VISIBLE,
This flag is only used in a test, why don't we just remove it.

https://codereview.chromium.org/1645843003/diff/300001/ui/message_center/mess...
ui/message_center/message_center.h:59: CLOSABLE,
These flag names are still confusing.  I don't have a great idea for naming, but
just reading the code where these flags are used I am not sure I would have an
accurate idea of what they actually do without reading the documentation here.

Possibly renaming the "closable" concept would be better.  Something like
"pinned" may convey the idea of keeping them around better?

https://codereview.chromium.org/1645843003/diff/300001/ui/message_center/view...
File ui/message_center/views/message_center_view.cc (right):

https://codereview.chromium.org/1645843003/diff/300001/ui/message_center/view...
ui/message_center/views/message_center_view.cc:275: true /* by_ser */,
message_center::MessageCenter::RemoveType::CLOSABLE);
nit: by_user

https://codereview.chromium.org/1645843003/diff/300001/ui/message_center/view...
File ui/message_center/views/message_list_view.cc (right):

https://codereview.chromium.org/1645843003/diff/300001/ui/message_center/view...
ui/message_center/views/message_list_view.cc:194: void
MessageListView::ClearAllClosableNotifications(
How does the clear-all animation look when there are some non-closable
notifications visible in the message center when you hit the clear all button? 
Since you skipp the closable one, you may need some more animations to signal
that this one is not getting closed.

https://codereview.chromium.org/1645843003/diff/300001/ui/views/controls/slid...
File ui/views/controls/slide_out_view.h (right):

https://codereview.chromium.org/1645843003/diff/300001/ui/views/controls/slid...
ui/views/controls/slide_out_view.h:24: bool slide_out_enable() { return
slide_out_enable_; }
nit: slide_out_enabled

[yoshiki, 2016-02-09 20:04:30]

Patchset #4 (id:340001) has been deleted

[yoshiki, 2016-02-09 20:04:44]

Patchset #3 (id:320001) has been deleted

[yoshiki, 2016-02-09 20:04:59]

Patchset #3 (id:360001) has been deleted

[yoshiki, 2016-02-09 23:38:20]

Description was changed from

==========
Implement Non-Closable Notification

This patch adds sticky notifications, which can't be removed by user from the
message center. This is only for ChromeOS, since the message center doesn't
exist on the other platforms. 

All toasts must be closable. But if the closing toast has non-closable
attribute, it'll be kept in the message center after closing. 

BUG=565005
R=dewittj@chromium.org
==========

to

==========
Implement Non-Closable Notification

This patch adds pinned notifications, which can't be removed by user from the
message center. This is only for ChromeOS, since the message center doesn't
exist on the other platforms. 

All toasts must be closable. But if the closing toast has non-closable
attribute, it'll be kept in the message center after closing. 

BUG=565005
R=dewittj@chromium.org
==========

[yoshiki, 2016-02-09 23:42:25]

Justin, PTAL

https://codereview.chromium.org/1645843003/diff/300001/ash/accelerators/accel...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/1645843003/diff/300001/ash/accelerators/accel...
ash/accelerators/accelerator_controller_unittest.cc:1523: false /* by_usr */,
message_center::MessageCenter::RemoveType::ALL);
On 2016/02/08 18:02:34, dewittj wrote:
> nit: by_user

Done.

https://codereview.chromium.org/1645843003/diff/300001/ui/message_center/mess...
File ui/message_center/message_center.h (right):

https://codereview.chromium.org/1645843003/diff/300001/ui/message_center/mess...
ui/message_center/message_center.h:54: // Remove all notifications (including
invisible ones and unclosable ones).
On 2016/02/08 18:02:34, dewittj wrote:
> nit: remove the parenthetical phrase - I think |all| is clear enough.

Done.

https://codereview.chromium.org/1645843003/diff/300001/ui/message_center/mess...
ui/message_center/message_center.h:57: VISIBLE,
On 2016/02/08 18:02:34, dewittj wrote:
> This flag is only used in a test, why don't we just remove it.

Removed. Done.

https://codereview.chromium.org/1645843003/diff/300001/ui/message_center/mess...
ui/message_center/message_center.h:59: CLOSABLE,
On 2016/02/08 18:02:34, dewittj wrote:
> These flag names are still confusing.  I don't have a great idea for naming,
but
> just reading the code where these flags are used I am not sure I would have an
> accurate idea of what they actually do without reading the documentation here.
> 
> Possibly renaming the "closable" concept would be better.  Something like
> "pinned" may convey the idea of keeping them around better?

Thank you for suggestion! Renamed this to "pinned".

https://codereview.chromium.org/1645843003/diff/300001/ui/message_center/view...
File ui/message_center/views/message_center_view.cc (right):

https://codereview.chromium.org/1645843003/diff/300001/ui/message_center/view...
ui/message_center/views/message_center_view.cc:275: true /* by_ser */,
message_center::MessageCenter::RemoveType::CLOSABLE);
On 2016/02/08 18:02:34, dewittj wrote:
> nit: by_user

Done.

https://codereview.chromium.org/1645843003/diff/300001/ui/message_center/view...
File ui/message_center/views/message_list_view.cc (right):

https://codereview.chromium.org/1645843003/diff/300001/ui/message_center/view...
ui/message_center/views/message_list_view.cc:194: void
MessageListView::ClearAllClosableNotifications(
On 2016/02/08 18:02:34, dewittj wrote:
> How does the clear-all animation look when there are some non-closable
> notifications visible in the message center when you hit the clear all button?

> Since you skipp the closable one, you may need some more animations to signal
> that this one is not getting closed.

Android dosen't have any animation for non-closable notifications. So I think
it's ok. WDYT?

https://codereview.chromium.org/1645843003/diff/300001/ui/views/controls/slid...
File ui/views/controls/slide_out_view.h (right):

https://codereview.chromium.org/1645843003/diff/300001/ui/views/controls/slid...
ui/views/controls/slide_out_view.h:24: bool slide_out_enable() { return
slide_out_enable_; }
On 2016/02/08 18:02:34, dewittj wrote:
> nit: slide_out_enabled

Done.

[dewittj, 2016-02-10 00:03:15]

This mostly lgtm, I'd prefer not to do the copy of the notification if
avoidable.  (see comment)

https://codereview.chromium.org/1645843003/diff/300001/ui/message_center/view...
File ui/message_center/views/message_list_view.cc (right):

https://codereview.chromium.org/1645843003/diff/300001/ui/message_center/view...
ui/message_center/views/message_list_view.cc:194: void
MessageListView::ClearAllClosableNotifications(
On 2016/02/09 23:42:25, yoshiki wrote:
> On 2016/02/08 18:02:34, dewittj wrote:
> > How does the clear-all animation look when there are some non-closable
> > notifications visible in the message center when you hit the clear all
button?
> 
> > Since you skipp the closable one, you may need some more animations to
signal
> > that this one is not getting closed.
> 
> Android dosen't have any animation for non-closable notifications. So I think
> it's ok. WDYT? 

Does the ChromeOS notification center close itself if clear-all is pressed with
a pinned notification, or does it remain open?

https://codereview.chromium.org/1645843003/diff/380001/ui/message_center/noti...
File ui/message_center/notification.h (right):

https://codereview.chromium.org/1645843003/diff/380001/ui/message_center/noti...
ui/message_center/notification.h:56: bool pinned;
please document this field somewhere, I realize that not all fields are
documented but this one may have non-obvious implications.

https://codereview.chromium.org/1645843003/diff/380001/ui/message_center/view...
File ui/message_center/views/message_popup_collection.cc (right):

https://codereview.chromium.org/1645843003/diff/380001/ui/message_center/view...
ui/message_center/views/message_popup_collection.cc:97: // Don't remove the
notification only when it's not closable.
nit: when it's pinned.

https://codereview.chromium.org/1645843003/diff/380001/ui/message_center/view...
ui/message_center/views/message_popup_collection.cc:160: Notification
notification = *(*iter);
I don't love copying the notification in order to set_pinned = false.  Should we
just use the top_level boolean to determine pinnability instead?

https://codereview.chromium.org/1645843003/diff/380001/ui/message_center/view...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/1645843003/diff/380001/ui/message_center/view...
ui/message_center/views/notification_view.cc:463:
controller_->RemoveNotification(notification_id(), true);  // By user.
nit: should be consistent in comments, so this should be true /* by user */

[dewittj, 2016-02-10 00:06:13]

+miguelg FYI

Please investigate how this interacts with the (behind a flag) Mac native
notification center integration.  Doesn't necessarily need to be done now but we
should at least hit a NOTIMPLEMENTED if we try to use this flag on unsupported
notification centers.

[Miguel Garcia, 2016-02-10 10:00:54]

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

[Miguel Garcia, 2016-02-10 10:00:54]

https://codereview.chromium.org/1645843003/diff/380001/ui/message_center/noti...
File ui/message_center/notification.h (right):

https://codereview.chromium.org/1645843003/diff/380001/ui/message_center/noti...
ui/message_center/notification.h:210: bool pinned() const { return
optional_fields_.pinned; }
Would it make sense to ifdef these methods of CrOs? I understand why this is
needed for CrOs but I don't think we will want them on other platforms.

[yoshiki, 2016-02-12 17:38:14]

Patchset #5 (id:420001) has been deleted

[yoshiki, 2016-02-12 23:11:36]

https://codereview.chromium.org/1645843003/diff/380001/ui/message_center/noti...
File ui/message_center/notification.h (right):

https://codereview.chromium.org/1645843003/diff/380001/ui/message_center/noti...
ui/message_center/notification.h:56: bool pinned;
On 2016/02/10 00:03:15, dewittj wrote:
> please document this field somewhere, I realize that not all fields are
> documented but this one may have non-obvious implications.

Done.

https://codereview.chromium.org/1645843003/diff/380001/ui/message_center/noti...
ui/message_center/notification.h:210: bool pinned() const { return
optional_fields_.pinned; }
On 2016/02/10 10:00:54, Miguel Garcia wrote:
> Would it make sense to ifdef these methods of CrOs? I understand why this is
> needed for CrOs but I don't think we will want them on other platforms.

It is a chromeos only feature. Done.

https://codereview.chromium.org/1645843003/diff/380001/ui/message_center/view...
File ui/message_center/views/message_popup_collection.cc (right):

https://codereview.chromium.org/1645843003/diff/380001/ui/message_center/view...
ui/message_center/views/message_popup_collection.cc:97: // Don't remove the
notification only when it's not closable.
On 2016/02/10 00:03:15, dewittj wrote:
> nit: when it's pinned. 

Done.

https://codereview.chromium.org/1645843003/diff/380001/ui/message_center/view...
ui/message_center/views/message_popup_collection.cc:160: Notification
notification = *(*iter);
On 2016/02/10 00:03:15, dewittj wrote:
> I don't love copying the notification in order to set_pinned = false.  Should
we
> just use the top_level boolean to determine pinnability instead?

Done.

[yoshiki, 2016-02-12 23:29:20]

Description was changed from

==========
Implement Non-Closable Notification

This patch adds pinned notifications, which can't be removed by user from the
message center. This is only for ChromeOS, since the message center doesn't
exist on the other platforms. 

All toasts must be closable. But if the closing toast has non-closable
attribute, it'll be kept in the message center after closing. 

BUG=565005
R=dewittj@chromium.org
==========

to

==========
Implement Non-Closable Notification

This patch adds pinned notifications, which can't be removed by user from the
message center. This feature is only for ChromeOS, since the message center
doesn't exist on the other platforms.

All toasts must be closable. But if the closing toast has non-closable
attribute, it'll be kept in the message center after closing. 

BUG=565005
R=dewittj@chromium.org
==========

[yoshiki, 2016-02-12 23:29:24]

yoshiki@chromium.org changed reviewers:
+ benwells@chromium.org, sky@chromium.org

[yoshiki, 2016-02-12 23:29:24]

sky@, benwells@, Could you take a look at following changes? Thanks.

sky@: ash/* and ui/views/*
benwells@: chrome/browser/ui/cocoa/apps/*

[benwells, 2016-02-15 06:13:35]

c/b/ui/cocoa/apps lgtm

[sky, 2016-02-16 16:13:06]

sky@chromium.org changed reviewers:
+ sadrul@chromium.org

[sky, 2016-02-16 16:13:06]

+sadrul for SlideOutView

The rest LGTM

https://codereview.chromium.org/1645843003/diff/460001/ui/views/controls/slid...
File ui/views/controls/slide_out_view.cc (right):

https://codereview.chromium.org/1645843003/diff/460001/ui/views/controls/slid...
ui/views/controls/slide_out_view.cc:13: bool SlideOutView::disable_animations_ =
false;
// static

https://codereview.chromium.org/1645843003/diff/460001/ui/views/controls/slid...
File ui/views/controls/slide_out_view.h (right):

https://codereview.chromium.org/1645843003/diff/460001/ui/views/controls/slid...
ui/views/controls/slide_out_view.h:19: static void
DisableAnimationsForTesting();
Constructor/destructor come before other functions.

https://codereview.chromium.org/1645843003/diff/460001/ui/views/controls/slid...
ui/views/controls/slide_out_view.h:55: bool slide_out_enabled_ = true;
is_slide_out_enabled_ (and rename functions to match).

I'm not really familiar with this class. I'm replacing myself with Sadrul, who I
believe refactored/wrote it.

[sadrul, 2016-02-17 17:29:26]

https://codereview.chromium.org/1645843003/diff/460001/ui/views/controls/slid...
File ui/views/controls/slide_out_view.cc (right):

https://codereview.chromium.org/1645843003/diff/460001/ui/views/controls/slid...
ui/views/controls/slide_out_view.cc:60: } else {
Define kScrollRatioForClosingNotification here.

https://codereview.chromium.org/1645843003/diff/460001/ui/views/controls/slid...
ui/views/controls/slide_out_view.cc:69: layer()->SetOpacity(1.f);
Do you need to SetOpacity() here at all?

https://codereview.chromium.org/1645843003/diff/460001/ui/views/controls/slid...
ui/views/controls/slide_out_view.cc:116: OnImplicitAnimationsCompleted();
Can you use ScopedAnimationDurationScaleMode with ZERO_DURATION for the test,
instead of having this disable_animations_?

https://codereview.chromium.org/1645843003/diff/460001/ui/views/controls/slid...
File ui/views/controls/slide_out_view.h (right):

https://codereview.chromium.org/1645843003/diff/460001/ui/views/controls/slid...
ui/views/controls/slide_out_view.h:19: static void
DisableAnimationsForTesting();
Can you move this into a *TestApi (see other similar things ui/views/test/)?

[yoshiki, 2016-02-22 13:39:27]

sadrul@, PTAL. Thanks.

https://codereview.chromium.org/1645843003/diff/460001/ui/views/controls/slid...
File ui/views/controls/slide_out_view.cc (right):

https://codereview.chromium.org/1645843003/diff/460001/ui/views/controls/slid...
ui/views/controls/slide_out_view.cc:60: } else {
On 2016/02/17 17:29:26, sadrul wrote:
> Define kScrollRatioForClosingNotification here.

We can't. kScrollRatioForClosingNotification is used l.79 as well.

https://codereview.chromium.org/1645843003/diff/460001/ui/views/controls/slid...
ui/views/controls/slide_out_view.cc:69: layer()->SetOpacity(1.f);
On 2016/02/17 17:29:26, sadrul wrote:
> Do you need to SetOpacity() here at all?

Removed

https://codereview.chromium.org/1645843003/diff/460001/ui/views/controls/slid...
ui/views/controls/slide_out_view.cc:116: OnImplicitAnimationsCompleted();
On 2016/02/17 17:29:26, sadrul wrote:
> Can you use ScopedAnimationDurationScaleMode with ZERO_DURATION for the test,
> instead of having this disable_animations_?

I didn't know that. I can remove the flag with ScopedAnimationDurationScaleMode.
Thanks.

https://codereview.chromium.org/1645843003/diff/460001/ui/views/controls/slid...
File ui/views/controls/slide_out_view.h (right):

https://codereview.chromium.org/1645843003/diff/460001/ui/views/controls/slid...
ui/views/controls/slide_out_view.h:55: bool slide_out_enabled_ = true;
On 2016/02/16 16:13:06, sky wrote:
> is_slide_out_enabled_ (and rename functions to match).
> 
> I'm not really familiar with this class. I'm replacing myself with Sadrul, who
I
> believe refactored/wrote it.

Done.

[yoshiki, 2016-02-25 01:45:18]

sky@, could you look at the SlideView change on behalf of sadrul@, or add other
appropriate owner, since he is ooo? Thanks.

[sky, 2016-02-25 02:35:41]

SlideOutView LGTM with the following

https://codereview.chromium.org/1645843003/diff/480001/ui/views/controls/slid...
File ui/views/controls/slide_out_view.h (right):

https://codereview.chromium.org/1645843003/diff/480001/ui/views/controls/slid...
ui/views/controls/slide_out_view.h:41: static bool disable_animations_;
Remove this as you no longer use/need it.

[yoshiki, 2016-02-25 04:46:27]

Thanks!

https://codereview.chromium.org/1645843003/diff/480001/ui/views/controls/slid...
File ui/views/controls/slide_out_view.h (right):

https://codereview.chromium.org/1645843003/diff/480001/ui/views/controls/slid...
ui/views/controls/slide_out_view.h:41: static bool disable_animations_;
On 2016/02/25 02:35:41, sky wrote:
> Remove this as you no longer use/need it.

Done.

[yoshiki, 2016-02-25 04:46:39]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2016-02-25 04:46:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
dewittj@chromium.org, benwells@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1645843003/#ps500001
(title: "Remove unnecessary property.")

[commit-bot: I haz the power, 2016-02-25 04:47:17]

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

[commit-bot: I haz the power, 2016-02-25 05:52:20]

Description was changed from

==========
Implement Non-Closable Notification

This patch adds pinned notifications, which can't be removed by user from the
message center. This feature is only for ChromeOS, since the message center
doesn't exist on the other platforms.

All toasts must be closable. But if the closing toast has non-closable
attribute, it'll be kept in the message center after closing. 

BUG=565005
R=dewittj@chromium.org
==========

to

==========
Implement Non-Closable Notification

This patch adds pinned notifications, which can't be removed by user from the
message center. This feature is only for ChromeOS, since the message center
doesn't exist on the other platforms.

All toasts must be closable. But if the closing toast has non-closable
attribute, it'll be kept in the message center after closing. 

BUG=565005
R=dewittj@chromium.org
==========

[commit-bot: I haz the power, 2016-02-25 05:52:21]

Committed patchset #8 (id:500001)

[commit-bot: I haz the power, 2016-02-25 05:53:50]

Description was changed from

==========
Implement Non-Closable Notification

This patch adds pinned notifications, which can't be removed by user from the
message center. This feature is only for ChromeOS, since the message center
doesn't exist on the other platforms.

All toasts must be closable. But if the closing toast has non-closable
attribute, it'll be kept in the message center after closing. 

BUG=565005
R=dewittj@chromium.org
==========

to

==========
Implement Non-Closable Notification

This patch adds pinned notifications, which can't be removed by user from the
message center. This feature is only for ChromeOS, since the message center
doesn't exist on the other platforms.

All toasts must be closable. But if the closing toast has non-closable
attribute, it'll be kept in the message center after closing.

BUG=565005
R=dewittj@chromium.org

Committed: https://crrev.com/35c9a3b689e257570a0ba08af1c6e0dc18ac3cc1
Cr-Commit-Position: refs/heads/master@{#377517}
==========

[commit-bot: I haz the power, 2016-02-25 05:53:51]

Patchset 8 (id:??) landed as
https://crrev.com/35c9a3b689e257570a0ba08af1c6e0dc18ac3cc1
Cr-Commit-Position: refs/heads/master@{#377517}