Show message center on lock screen

Show message center on lock screen

This patch does:
- Show the message center button on tray on the lock screen
- Show the message to encourage user to log in when user opens message center on lock screen
- Add SetLockedState(bool) and IsLockedState() method into MessageCenter class.
- Don't make notifications read when opening the message center on lock screen
- Add tests

And this patch also fixes the bug that this makes blocked notifications read wrongly.

BUG=599891
TEST=manually tested

Committed: https://crrev.com/48c4b94a9d9eb0ae3a20ed9c05d9e77ff1a98b50
Cr-Commit-Position: refs/heads/master@{#396402}

[yoshiki, 2016-05-16 18:49:08]

Patchset #1 (id:1) has been deleted

[yoshiki, 2016-05-16 18:50:55]

Patchset #2 (id:40001) has been deleted

[yoshiki, 2016-05-16 18:54:22]

Patchset #1 (id:20001) has been deleted

[yoshiki, 2016-05-16 19:04:00]

Description was changed from

==========
.

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

to

==========
Show message center on lock screen

This patch does:
- Show the message center button on tray on the lock screen
- Show the message to encourage user to log in when user opens message center on
lock screen
- Add SetLockedState(bool) and IsLockedState() method into MessageCenter class.
- Don't make notifications read when opening the message center on lock screen
- Add tests

And this patch also fixes the bug that this makes blocked notifications read
wrongly.

BUG=599891
TEST=manually tested
==========

[yoshiki, 2016-05-16 19:04:00]

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

[yoshiki, 2016-05-16 19:04:47]

Justin, PTAL. Thanks.

https://codereview.chromium.org/1986493002/diff/60001/ui/message_center/notif...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/1986493002/diff/60001/ui/message_center/notif...
ui/message_center/notification_list.cc:70: void
NotificationList::SetNotificationsShown(
I splitted this method because I don't want to make notifications read when the
screen is locked.

https://codereview.chromium.org/1986493002/diff/60001/ui/message_center/notif...
ui/message_center/notification_list.cc:71: const NotificationBlockers& blockers,
Adding blockers fixes the bug that this makes blocked notifications read.

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

https://codereview.chromium.org/1986493002/diff/60001/ui/message_center/views...
ui/message_center/views/message_center_view_unittest.cc:219: widget_.reset(new
views::Widget());
Widget is necessary to use View::IsDrawn().

[yoshiki, 2016-05-19 00:33:48]

Justin, PTAL. Thanks.

[dewittj, 2016-05-19 17:34:03]

I'm wondering if there is a way to do this patch without teaching the message
center about the locked state.  Isn't that the job of the bubble?  Should we
expose MessageCenterButtonBar separately and allow the bubble to use that?

https://codereview.chromium.org/1986493002/diff/60001/ui/message_center/messa...
File ui/message_center/message_center.h (right):

https://codereview.chromium.org/1986493002/diff/60001/ui/message_center/messa...
ui/message_center/message_center.h:175: // This can be called to change the
quiet mode state (without a timeout).
nit: no need to add timeout to comment.

https://codereview.chromium.org/1986493002/diff/60001/ui/message_center/notif...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/1986493002/diff/60001/ui/message_center/notif...
ui/message_center/notification_list.cc:67: message_center_visible_ = visible;
This doesn't seem like the right place to track this bit anymore.

https://codereview.chromium.org/1986493002/diff/60001/ui/message_center/notif...
File ui/message_center/notification_list.h (right):

https://codereview.chromium.org/1986493002/diff/60001/ui/message_center/notif...
ui/message_center/notification_list.h:62: void SetMessageCenterVisible(bool
visible);
please add comment about what this does

[yoshiki, 2016-05-24 16:52:16]

Patchset #2 (id:80001) has been deleted

[yoshiki, 2016-05-24 16:56:08]

Patchset #2 (id:100001) has been deleted

[yoshiki, 2016-05-24 17:00:14]

In near future, we're planning to show some non-private notifications on lock
screen (like Android). So I think it's better to teach the message center about
the locked state. WDYT?

https://codereview.chromium.org/1986493002/diff/60001/ui/message_center/messa...
File ui/message_center/message_center.h (right):

https://codereview.chromium.org/1986493002/diff/60001/ui/message_center/messa...
ui/message_center/message_center.h:175: // This can be called to change the
quiet mode state (without a timeout).
On 2016/05/19 17:34:03, dewittj wrote:
> nit: no need to add timeout to comment.

Done.

https://codereview.chromium.org/1986493002/diff/60001/ui/message_center/notif...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/1986493002/diff/60001/ui/message_center/notif...
ui/message_center/notification_list.cc:67: message_center_visible_ = visible;
On 2016/05/19 17:34:03, dewittj wrote:
> This doesn't seem like the right place to track this bit anymore.

I moved the flag from NotificationList to MessageCenter. I think it's more
natural.

https://codereview.chromium.org/1986493002/diff/60001/ui/message_center/notif...
File ui/message_center/notification_list.h (right):

https://codereview.chromium.org/1986493002/diff/60001/ui/message_center/notif...
ui/message_center/notification_list.h:62: void SetMessageCenterVisible(bool
visible);
On 2016/05/19 17:34:03, dewittj wrote:
> please add comment about what this does

Removed this method.

[dewittj, 2016-05-24 17:10:29]

ok, lgtm

https://codereview.chromium.org/1986493002/diff/120001/ui/message_center/noti...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/1986493002/diff/120001/ui/message_center/noti...
ui/message_center/notification_list.cc:343:
notification->set_shown_as_popup(message_center_->IsMessageCenterVisible()
This is ok for now, but I'd rather the notification already get its
shown_as_popup bit set before it's put onto the notification list one eday.

[yoshiki, 2016-05-25 09:13:59]

https://codereview.chromium.org/1986493002/diff/120001/ui/message_center/noti...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/1986493002/diff/120001/ui/message_center/noti...
ui/message_center/notification_list.cc:343:
notification->set_shown_as_popup(message_center_->IsMessageCenterVisible()
On 2016/05/24 17:10:29, dewittj wrote:
> This is ok for now, but I'd rather the notification already get its
> shown_as_popup bit set before it's put onto the notification list one eday.

Got it. Let me consider a good way.

[yoshiki, 2016-05-25 09:14:46]

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

[yoshiki, 2016-05-25 09:14:47]

skuhne@, could you approve the change in ash/system/*? Thanks.

[Mr4D (OOO till 08-26), 2016-05-25 16:45:15]

Which message indicator gets shown in multi user mode?

Is it guaranteed that the unlocking user's message indicator gets shown (note
that that is not necessarily the user which was active before the system got
locked)?

[yoshiki, 2016-05-26 14:04:01]

On 2016/05/25 16:45:15, Mr4D wrote:
> Which message indicator gets shown in multi user mode?
> 
> Is it guaranteed that the unlocking user's message indicator gets shown (note
> that that is not necessarily the user which was active before the system got
> locked)?

On lock screen, the button shows the unread count of the user which was active
before the system got locked. This is because an user change event is not
received at this time. So entire chrome OS keeps the previous user's context
(eg. the avatar in the status tray, screenshot destination directory, locale,
...).

After unlocking, it shows the unread count of the user which unlocked (and the
desktop also changes to that user). Because an user change event is sent just
after unlocking.

[Mr4D (OOO till 08-26), 2016-05-26 15:24:47]

On 2016/05/26 14:04:01, yoshiki wrote:
> On 2016/05/25 16:45:15, Mr4D wrote:
> > Which message indicator gets shown in multi user mode?
> > 
> > Is it guaranteed that the unlocking user's message indicator gets shown
(note
> > that that is not necessarily the user which was active before the system got
> > locked)?
> 
> On lock screen, the button shows the unread count of the user which was active
> before the system got locked. This is because an user change event is not
> received at this time. So entire chrome OS keeps the previous user's context
> (eg. the avatar in the status tray, screenshot destination directory, locale,
> ...).
> 
> After unlocking, it shows the unread count of the user which unlocked (and the
> desktop also changes to that user). Because an user change event is sent just
> after unlocking.

That is what I have feared somehow. The problem I see with this is that upon
unlock 
you see the login screen of the primary user (which is part of the policy).
So if you see the wallpaper of that user + the login symbol of that user it is a
bit confusing that the message notifications are for the other user which was
visible
pre-lock.
I suppose that the tiny user icon inside the system area of the shelf must be
the
indicator to point this out. Ohhkay.

[Mr4D (OOO till 08-26), 2016-05-26 15:25:23]

ash/system/* lgtm

[yoshiki, 2016-05-27 03:37:56]

On 2016/05/26 15:24:47, Mr4D wrote:
> On 2016/05/26 14:04:01, yoshiki wrote:
> > On 2016/05/25 16:45:15, Mr4D wrote:
> > > Which message indicator gets shown in multi user mode?
> > > 
> > > Is it guaranteed that the unlocking user's message indicator gets shown
> (note
> > > that that is not necessarily the user which was active before the system
got
> > > locked)?
> > 
> > On lock screen, the button shows the unread count of the user which was
active
> > before the system got locked. This is because an user change event is not
> > received at this time. So entire chrome OS keeps the previous user's context
> > (eg. the avatar in the status tray, screenshot destination directory,
locale,
> > ...).
> > 
> > After unlocking, it shows the unread count of the user which unlocked (and
the
> > desktop also changes to that user). Because an user change event is sent
just
> > after unlocking.
> 
> That is what I have feared somehow. The problem I see with this is that upon
> unlock 
> you see the login screen of the primary user (which is part of the policy).
> So if you see the wallpaper of that user + the login symbol of that user it is
a
> bit confusing that the message notifications are for the other user which was
> visible
> pre-lock.
> I suppose that the tiny user icon inside the system area of the shelf must be
> the
> indicator to point this out. Ohhkay.

Thanks. Let me reconsider UI with PM&UX until showing some notifications in lock
screen.

[yoshiki, 2016-05-27 03:38:00]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2016-05-27 03:38:25]

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

[commit-bot: I haz the power, 2016-05-27 05:50:00]

Description was changed from

==========
Show message center on lock screen

This patch does:
- Show the message center button on tray on the lock screen
- Show the message to encourage user to log in when user opens message center on
lock screen
- Add SetLockedState(bool) and IsLockedState() method into MessageCenter class.
- Don't make notifications read when opening the message center on lock screen
- Add tests

And this patch also fixes the bug that this makes blocked notifications read
wrongly.

BUG=599891
TEST=manually tested
==========

to

==========
Show message center on lock screen

This patch does:
- Show the message center button on tray on the lock screen
- Show the message to encourage user to log in when user opens message center on
lock screen
- Add SetLockedState(bool) and IsLockedState() method into MessageCenter class.
- Don't make notifications read when opening the message center on lock screen
- Add tests

And this patch also fixes the bug that this makes blocked notifications read
wrongly.

BUG=599891
TEST=manually tested
==========

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

Committed patchset #2 (id:120001)

[commit-bot: I haz the power, 2016-05-27 05:52:09]

Description was changed from

==========
Show message center on lock screen

This patch does:
- Show the message center button on tray on the lock screen
- Show the message to encourage user to log in when user opens message center on
lock screen
- Add SetLockedState(bool) and IsLockedState() method into MessageCenter class.
- Don't make notifications read when opening the message center on lock screen
- Add tests

And this patch also fixes the bug that this makes blocked notifications read
wrongly.

BUG=599891
TEST=manually tested
==========

to

==========
Show message center on lock screen

This patch does:
- Show the message center button on tray on the lock screen
- Show the message to encourage user to log in when user opens message center on
lock screen
- Add SetLockedState(bool) and IsLockedState() method into MessageCenter class.
- Don't make notifications read when opening the message center on lock screen
- Add tests

And this patch also fixes the bug that this makes blocked notifications read
wrongly.

BUG=599891
TEST=manually tested

Committed: https://crrev.com/48c4b94a9d9eb0ae3a20ed9c05d9e77ff1a98b50
Cr-Commit-Position: refs/heads/master@{#396402}
==========

[commit-bot: I haz the power, 2016-05-27 05:52:10]

Patchset 2 (id:??) landed as
https://crrev.com/48c4b94a9d9eb0ae3a20ed9c05d9e77ff1a98b50
Cr-Commit-Position: refs/heads/master@{#396402}

[nednguyen, 2016-05-27 17:07:11]

A revert of this CL (patchset #2 id:120001) has been created in
https://codereview.chromium.org/2018063002/ by nednguyen@google.com.

The reason for reverting is: Speculative revert: this may cause
message_center_unittests failure on Ubuntu-12.04	

BUG=615443.

[yoshiki, 2016-05-30 06:44:21]

Description was changed from

==========
Show message center on lock screen

This patch does:
- Show the message center button on tray on the lock screen
- Show the message to encourage user to log in when user opens message center on
lock screen
- Add SetLockedState(bool) and IsLockedState() method into MessageCenter class.
- Don't make notifications read when opening the message center on lock screen
- Add tests

And this patch also fixes the bug that this makes blocked notifications read
wrongly.

BUG=599891
TEST=manually tested

Committed: https://crrev.com/48c4b94a9d9eb0ae3a20ed9c05d9e77ff1a98b50
Cr-Commit-Position: refs/heads/master@{#396402}
==========

to

==========
Show message center on lock screen

This patch does:
- Show the message center button on tray on the lock screen
- Show the message to encourage user to log in when user opens message center on
lock screen
- Add SetLockedState(bool) and IsLockedState() method into MessageCenter class.
- Don't make notifications read when opening the message center on lock screen
- Add tests

And this patch also fixes the bug that this makes blocked notifications read
wrongly.

BUG=599891
TEST=manually tested

Committed: https://crrev.com/48c4b94a9d9eb0ae3a20ed9c05d9e77ff1a98b50
Cr-Commit-Position: refs/heads/master@{#396402}
==========