mash: Convert WebNotificationTray to wm common types

mash: Convert WebNotificationTray to wm common types

This is a dependency of the status area widget which I'm trying to convert to
wm common types.

Introduce a method to the message center's delegate that allows it to
compute the parent container for a popup. This cannot be a NativeView, since
on mus a NativeView is an aura::Window, but the parent container is a mus
window.

This can't move to //ash/common yet because of system tray dependencies.

BUG=619636
TEST=ash_unittests

Committed: https://crrev.com/21c95c56e86bda6f8f056551ca9ee043317ed1d6
Cr-Commit-Position: refs/heads/master@{#400738}

[James Cook, 2016-06-17 21:24:13]

jamescook@chromium.org changed reviewers:
+ msw@chromium.org

[James Cook, 2016-06-17 21:24:14]

msw, please take a look.

[msw, 2016-06-17 22:28:11]

A bit complex, but seems reasonable. lgtm with a q.

https://codereview.chromium.org/2075953003/diff/20001/ash/system/web_notifica...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2075953003/diff/20001/ash/system/web_notifica...
ash/system/web_notification/web_notification_tray.cc:506: return
status_area_widget()->login_status() != LoginStatus::NOT_LOGGED_IN &&
q: Is it odd that this doesn't check for LoginStatus::LOCKED, etc?

[James Cook, 2016-06-17 23:46:33]

jamescook@chromium.org changed reviewers:
+ stevenjb@chromium.org

[James Cook, 2016-06-17 23:46:33]

stevenjb, can I get OWNERS for ui/message_center ?

(Also, any idea on the question below?)

https://codereview.chromium.org/2075953003/diff/20001/ash/system/web_notifica...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2075953003/diff/20001/ash/system/web_notifica...
ash/system/web_notification/web_notification_tray.cc:506: return
status_area_widget()->login_status() != LoginStatus::NOT_LOGGED_IN &&
On 2016/06/17 22:28:11, msw wrote:
> q: Is it odd that this doesn't check for LoginStatus::LOCKED, etc?

A little, but maybe you want notifications at the lock screen? I think we might
reuse these sorts of notifications for things like "screen extended to monitor".
Of course, that begs the question why we don't do it when adding a user.

It is odd, but in the absence of better understanding I don't want to change it.

[stevenjb, 2016-06-20 17:41:35]

lgtm

https://codereview.chromium.org/2075953003/diff/20001/ash/system/web_notifica...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2075953003/diff/20001/ash/system/web_notifica...
ash/system/web_notification/web_notification_tray.cc:506: return
status_area_widget()->login_status() != LoginStatus::NOT_LOGGED_IN &&
On 2016/06/17 23:46:33, James Cook wrote:
> On 2016/06/17 22:28:11, msw wrote:
> > q: Is it odd that this doesn't check for LoginStatus::LOCKED, etc?
> 
> A little, but maybe you want notifications at the lock screen? I think we
might
> reuse these sorts of notifications for things like "screen extended to
monitor".
> Of course, that begs the question why we don't do it when adding a user.
> 
> It is odd, but in the absence of better understanding I don't want to change
it.

Apparently elsewhere we hide the message center when the screen is locked.

FWIW, I don't think "LOCKED" should be part of LoginStatus, the concepts are at
least partly orthogonal. (And really I don't think we need/want
status_area_widget()->login_status() anymore, iirc it predates LoginState).

I also don't think this class should need to be aware of SessionStateDelegate
(which I'm not actually familiar with).

That said, i wouldn't change the logic in this CL.

[James Cook, 2016-06-20 18:05:10]

Thanks for the reviews, guys.

I may need to take login status out of StatusAreaWidget in the future, but I'm
not going to mess with it here.

https://codereview.chromium.org/2075953003/diff/20001/ash/system/web_notifica...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2075953003/diff/20001/ash/system/web_notifica...
ash/system/web_notification/web_notification_tray.cc:506: return
status_area_widget()->login_status() != LoginStatus::NOT_LOGGED_IN &&
On 2016/06/20 17:41:35, stevenjb wrote:
> On 2016/06/17 23:46:33, James Cook wrote:
> > On 2016/06/17 22:28:11, msw wrote:
> > > q: Is it odd that this doesn't check for LoginStatus::LOCKED, etc?
> > 
> > A little, but maybe you want notifications at the lock screen? I think we
> might
> > reuse these sorts of notifications for things like "screen extended to
> monitor".
> > Of course, that begs the question why we don't do it when adding a user.
> > 
> > It is odd, but in the absence of better understanding I don't want to change
> it.
> 
> Apparently elsewhere we hide the message center when the screen is locked.
> 
> FWIW, I don't think "LOCKED" should be part of LoginStatus, the concepts are
at
> least partly orthogonal. (And really I don't think we need/want
> status_area_widget()->login_status() anymore, iirc it predates LoginState).
> 
> I also don't think this class should need to be aware of SessionStateDelegate
> (which I'm not actually familiar with).
> 
> That said, i wouldn't change the logic in this CL.
> 

Acknowledged.

[James Cook, 2016-06-20 18:05:23]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-06-20 18:05:23]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2075953003/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-20 18:06:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2075953003/80001

[commit-bot: I haz the power, 2016-06-20 18:47:34]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-20 18:50:25]

Description was changed from

==========
mash: Convert WebNotificationTray to wm common types

This is a dependency of the status area widget which I'm trying to convert to
wm common types.

Introduce a method to the message center's delegate that allows it to
compute the parent container for a popup. This cannot be a NativeView, since
on mus a NativeView is an aura::Window, but the parent container is a mus
window.

This can't move to //ash/common yet because of system tray dependencies.

BUG=619636
TEST=ash_unittests
==========

to

==========
mash: Convert WebNotificationTray to wm common types

This is a dependency of the status area widget which I'm trying to convert to
wm common types.

Introduce a method to the message center's delegate that allows it to
compute the parent container for a popup. This cannot be a NativeView, since
on mus a NativeView is an aura::Window, but the parent container is a mus
window.

This can't move to //ash/common yet because of system tray dependencies.

BUG=619636
TEST=ash_unittests

Committed: https://crrev.com/21c95c56e86bda6f8f056551ca9ee043317ed1d6
Cr-Commit-Position: refs/heads/master@{#400738}
==========

[commit-bot: I haz the power, 2016-06-20 18:50:26]

Patchset 5 (id:??) landed as
https://crrev.com/21c95c56e86bda6f8f056551ca9ee043317ed1d6
Cr-Commit-Position: refs/heads/master@{#400738}