Don't show system notifications while the screen is locked.

Don't show system notifications while the screen is locked.

Change-Id: Ie6079866fb0b957e56aff2993745e8976c4fb10b

BUG=124402
TEST=Test system notifications (power, network, sms) especially around screen lock/unlock.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=137159

[stevenjb, 2012-05-11 00:30:44]

This is an attempt to fix
http://code.google.com/p/chromium/issues/detail?id=124402. I'm pretty certain it
is related to showing the notification while the screen is locked (but the bug
is difficult to reproduce).

[Daniel Erat, 2012-05-11 14:12:38]

http://codereview.chromium.org/10382118/diff/1/chrome/browser/chromeos/notifi...
File chrome/browser/chromeos/notifications/system_notification.cc (right):

http://codereview.chromium.org/10382118/diff/1/chrome/browser/chromeos/notifi...
chrome/browser/chromeos/notifications/system_notification.cc:20:
DBusThreadManager::Get()->GetPowerManagerClient()->GetIsScreenLocked();
i don't see this method at
git.chromium.org/gitweb/?p=chromium.git;a=blob;f=chromeos/dbus/power_manager_client.h;h=00c27b83007f785056948c878fbfc241e531e300;hb=HEAD
-- am i looking in the wrong place?

[stevenjb, 2012-05-11 16:34:46]

Add GetIsScreenLocked to PowerManagerClient

[stevenjb, 2012-05-11 16:35:29]

ptal

http://codereview.chromium.org/10382118/diff/1/chrome/browser/chromeos/notifi...
File chrome/browser/chromeos/notifications/system_notification.cc (right):

http://codereview.chromium.org/10382118/diff/1/chrome/browser/chromeos/notifi...
chrome/browser/chromeos/notifications/system_notification.cc:20:
DBusThreadManager::Get()->GetPowerManagerClient()->GetIsScreenLocked();
On 2012/05/11 14:12:38, Daniel Erat wrote:
> i don't see this method at
>
git.chromium.org/gitweb/?p=chromium.git;a=blob;f=chromeos/dbus/power_manager_client.h;h=00c27b83007f785056948c878fbfc241e531e300;hb=HEAD
> -- am i looking in the wrong place?

Doh! Forgot to add those files...

[Daniel Erat, 2012-05-11 17:16:18]

Summarizing in-person discussion: I'm pretty sure that this is happening because
browser/ui/views/notifications/balloon_view.cc calls
Widget::GetWindowScreenBounds() on the notification frame and then uses that to
find the position for the notification content.  When the screen is locked,
everything's transformed and scaled to be squished into the center of the
screen, so the notification ends up getting placed there (the Aura impl of
GetWindowScreenBounds() applies transformations).

The right fix is probably to make the notification content get parented to the
frame (although maybe there's a reason it wasn't done that way in the first
place).  Per Steven, it sounds like this code will be replaced soon, so taking
the approach in the current version of the change (but maybe just deferring the
creation of notifications that are requested while the screen is locked, rather
than hiding/unhiding existing ones when the lock state changes) sounds okay to
me.

[oshima, 2012-05-11 17:36:49]

On 2012/05/11 17:16:18, Daniel Erat wrote:
> Summarizing in-person discussion: I'm pretty sure that this is happening
because
> browser/ui/views/notifications/balloon_view.cc calls
> Widget::GetWindowScreenBounds() on the notification frame and then uses that
to
> find the position for the notification content.  When the screen is locked,
> everything's transformed and scaled to be squished into the center of the
> screen, so the notification ends up getting placed there (the Aura impl of
> GetWindowScreenBounds() applies transformations).

Does it? Where I can find the code (since this may cause other issues).

> 
> The right fix is probably to make the notification content get parented to the
> frame (although maybe there's a reason it wasn't done that way in the first
> place).  Per Steven, it sounds like this code will be replaced soon, so taking
> the approach in the current version of the change (but maybe just deferring
the
> creation of notifications that are requested while the screen is locked,
rather
> than hiding/unhiding existing ones when the lock state changes) sounds okay to
> me.

Or just re-position ballons. Ballons should be re-positioned when the screen
size changes,
and if we already do it, we can just use it i guess? If we don't currently, then
never mind.

[stevenjb, 2012-05-11 18:33:54]

Rebase

[stevenjb, 2012-05-11 18:40:16]

Only defer notifications on screen lock (don't hide)

[stevenjb, 2012-05-11 18:43:44]

This change now only defers showing a notification if it is shown during screen
lock; it no longer hides on screen lock. This should have no affect except in
the case that is failing and thus should be safer.

I'm not sure if there is an easy way to just reliably update notifications after
a screen lock, so this seemed like the simplest solution, especially since it
will only last for one release.

[oshima, 2012-05-11 19:15:46]

By the way, what about regular notifications such as email/calendar
notifications? Won't they have the same issue?

On 2012/05/11 18:43:44, stevenjb (chromium) wrote:
> This change now only defers showing a notification if it is shown during
screen
> lock; it no longer hides on screen lock. This should have no affect except in
> the case that is failing and thus should be safer.
> 
> I'm not sure if there is an easy way to just reliably update notifications
after
> a screen lock, so this seemed like the simplest solution, especially since it
> will only last for one release.

[stevenjb, 2012-05-11 19:22:50]

Maybe? I haven't seen a bug for that though. Perhaps they are less susceptible
to getting created when the screen locker is up?
We're hoping to replace desktop notifications entirely for Ash in R21...

[oshima, 2012-05-11 19:40:07]

i'm ok with the approach. Just one question.

http://codereview.chromium.org/10382118/diff/6003/chrome/browser/chromeos/not...
File chrome/browser/chromeos/notifications/system_notification.cc (right):

http://codereview.chromium.org/10382118/diff/6003/chrome/browser/chromeos/not...
chrome/browser/chromeos/notifications/system_notification.cc:87: if
(DBusThreadManager::Get()->GetPowerManagerClient()->GetIsScreenLocked()) {
can you use ScreenLocker::default_screen_locker()->locked() ?

[stevenjb, 2012-05-11 20:19:10]

On 2012/05/11 19:40:07, oshima wrote:
> i'm ok with the approach. Just one question.
> 
>
http://codereview.chromium.org/10382118/diff/6003/chrome/browser/chromeos/not...
> File chrome/browser/chromeos/notifications/system_notification.cc (right):
> 
>
http://codereview.chromium.org/10382118/diff/6003/chrome/browser/chromeos/not...
> chrome/browser/chromeos/notifications/system_notification.cc:87: if
> (DBusThreadManager::Get()->GetPowerManagerClient()->GetIsScreenLocked()) {
> can you use ScreenLocker::default_screen_locker()->locked() ?

Because we depend on the Unlock() callback to show the notification, it seemed
better to match the PowerManagerClient(), which may not be exactly the same as
ScreenLocker::default_screen_locker()->locked().

[Daniel Erat, 2012-05-11 20:20:24]

On Fri, May 11, 2012 at 10:36 AM,  <oshima@chromium.org> wrote:
> On 2012/05/11 17:16:18, Daniel Erat wrote:
>>
>> Summarizing in-person discussion: I'm pretty sure that this is happening
>
> because
>>
>> browser/ui/views/notifications/balloon_view.cc calls
>> Widget::GetWindowScreenBounds() on the notification frame and then uses
>> that
>
> to
>>
>> find the position for the notification content.  When the screen is
>> locked,
>> everything's transformed and scaled to be squished into the center of the
>> screen, so the notification ends up getting placed there (the Aura impl of
>> GetWindowScreenBounds() applies transformations).
>
>
> Does it? Where I can find the code (since this may cause other issues).

NativeWidgetAura::GetWindowScreenBounds() ->
aura::Window::GetBoundsInRootWindow() ->
aura::Window::ConvertPointToWindow() ->
ui::Layer::ConvertPointToLayer() ->
ui::Layer::Layer::ConvertPointFor/FromAncestor(), which look like they
include transforms between the two layers.

The code that does the transforms when the screen is locked or the
system is shutting down is at ash/wm/power_button_controller.cc.  I
suspect that any code that calls GetWindowScreenBounds() while the
screen is locked might end up with a point at the center of the
screen.

>> The right fix is probably to make the notification content get parented to
>> the
>> frame (although maybe there's a reason it wasn't done that way in the
>> first
>> place).  Per Steven, it sounds like this code will be replaced soon, so
>> taking
>> the approach in the current version of the change (but maybe just
>> deferring
>
> the
>>
>> creation of notifications that are requested while the screen is locked,
>
> rather
>>
>> than hiding/unhiding existing ones when the lock state changes) sounds
>> okay to
>> me.
>
>
> Or just re-position ballons. Ballons should be re-positioned when the screen
> size changes,
> and if we already do it, we can just use it i guess? If we don't currently,
> then
> never mind.
>
> http://codereview.chromium.org/10382118/

[oshima, 2012-05-11 20:25:04]

LGTM

We discussed offline and agreed that we should change places that uses
ScreenLocker::default_screen_locker()->locked() to use new API instead. Steven,
can you update them (in separate CL)? I think there are only a few places.

[Daniel Erat, 2012-05-11 20:25:32]

lgtm

[oshima, 2012-05-11 20:30:54]

On 2012/05/11 20:20:24, Daniel Erat wrote:
> On Fri, May 11, 2012 at 10:36 AM,  <mailto:oshima@chromium.org> wrote:
> > On 2012/05/11 17:16:18, Daniel Erat wrote:
> >>
> >> Summarizing in-person discussion: I'm pretty sure that this is happening
> >
> > because
> >>
> >> browser/ui/views/notifications/balloon_view.cc calls
> >> Widget::GetWindowScreenBounds() on the notification frame and then uses
> >> that
> >
> > to
> >>
> >> find the position for the notification content. &nbsp;When the screen is
> >> locked,
> >> everything's transformed and scaled to be squished into the center of the
> >> screen, so the notification ends up getting placed there (the Aura impl of
> >> GetWindowScreenBounds() applies transformations).
> >
> >
> > Does it? Where I can find the code (since this may cause other issues).
> 
> NativeWidgetAura::GetWindowScreenBounds() ->
> aura::Window::GetBoundsInRootWindow() ->
> aura::Window::ConvertPointToWindow() ->
> ui::Layer::ConvertPointToLayer() ->
> ui::Layer::Layer::ConvertPointFor/FromAncestor(), which look like they
> include transforms between the two layers.

Yes, I knew this one.

> The code that does the transforms when the screen is locked or the
> system is shutting down is at ash/wm/power_button_controller.cc.  I
> suspect that any code that calls GetWindowScreenBounds() while the
> screen is locked might end up with a point at the center of the
> screen.

This was my question and I now see the issue. I think including root's transform
is wrong here because
this should return the bounds relative to the root, not to the host window.

> >> The right fix is probably to make the notification content get parented to
> >> the
> >> frame (although maybe there's a reason it wasn't done that way in the
> >> first
> >> place). &nbsp;Per Steven, it sounds like this code will be replaced soon,
so
> >> taking
> >> the approach in the current version of the change (but maybe just
> >> deferring
> >
> > the
> >>
> >> creation of notifications that are requested while the screen is locked,
> >
> > rather
> >>
> >> than hiding/unhiding existing ones when the lock state changes) sounds
> >> okay to
> >> me.
> >
> >
> > Or just re-position ballons. Ballons should be re-positioned when the screen
> > size changes,
> > and if we already do it, we can just use it i guess? If we don't currently,
> > then
> > never mind.
> >
> > http://codereview.chromium.org/10382118/

[oshima, 2012-05-11 20:41:00]

On 2012/05/11 20:30:54, oshima wrote:
> On 2012/05/11 20:20:24, Daniel Erat wrote:
> > On Fri, May 11, 2012 at 10:36 AM,  <mailto:oshima@chromium.org> wrote:
> > > On 2012/05/11 17:16:18, Daniel Erat wrote:
> > >>
> > >> Summarizing in-person discussion: I'm pretty sure that this is happening
> > >
> > > because
> > >>
> > >> browser/ui/views/notifications/balloon_view.cc calls
> > >> Widget::GetWindowScreenBounds() on the notification frame and then uses
> > >> that
> > >
> > > to
> > >>
> > >> find the position for the notification content. &nbsp;When the screen is
> > >> locked,
> > >> everything's transformed and scaled to be squished into the center of the
> > >> screen, so the notification ends up getting placed there (the Aura impl
of
> > >> GetWindowScreenBounds() applies transformations).
> > >
> > >
> > > Does it? Where I can find the code (since this may cause other issues).
> > 
> > NativeWidgetAura::GetWindowScreenBounds() ->
> > aura::Window::GetBoundsInRootWindow() ->
> > aura::Window::ConvertPointToWindow() ->
> > ui::Layer::ConvertPointToLayer() ->
> > ui::Layer::Layer::ConvertPointFor/FromAncestor(), which look like they
> > include transforms between the two layers.
> 
> Yes, I knew this one.
> 
> > The code that does the transforms when the screen is locked or the
> > system is shutting down is at ash/wm/power_button_controller.cc.  I
> > suspect that any code that calls GetWindowScreenBounds() while the
> > screen is locked might end up with a point at the center of the
> > screen.
> 
> This was my question and I now see the issue. I think including root's
transform
> is wrong here because
> this should return the bounds relative to the root, not to the host window.

Sorry this was my misunderstanding. It was container that are shrunk, and that's
why
it couldn't get the right offset. Thanks Dan for correction.

> 
> > >> The right fix is probably to make the notification content get parented
to
> > >> the
> > >> frame (although maybe there's a reason it wasn't done that way in the
> > >> first
> > >> place). &nbsp;Per Steven, it sounds like this code will be replaced soon,
> so
> > >> taking
> > >> the approach in the current version of the change (but maybe just
> > >> deferring
> > >
> > > the
> > >>
> > >> creation of notifications that are requested while the screen is locked,
> > >
> > > rather
> > >>
> > >> than hiding/unhiding existing ones when the lock state changes) sounds
> > >> okay to
> > >> me.
> > >
> > >
> > > Or just re-position ballons. Ballons should be re-positioned when the
screen
> > > size changes,
> > > and if we already do it, we can just use it i guess? If we don't
currently,
> > > then
> > > never mind.
> > >
> > > http://codereview.chromium.org/10382118/

[stevenjb, 2012-05-14 22:37:11]

Rebase

[stevenjb, 2012-05-14 22:59:51]

Add GetIsScreenLocked to mock

[stevenjb, 2012-05-15 00:03:08]

Fix shutdown crash in tests.

[stevenjb, 2012-05-15 01:38:35]

Fix shutdown crash in tests.