mash: Convert system tray logout button to wm common types

mash: Convert system tray logout button to wm common types

* Fold LastWindowClosedLogoutReminder into LogoutConfirmationController
* Move LogoutConfirmationController ownership from Shell to WmShell
* Place the logout confirmation dialog in the system modal container, not
in the root (dialogs should not live directly in the root)
* Move LabelTrayView to //ash/common/system/tray since it is not chromeos-
specific

BUG=619636
TEST=ash_unittests, manual testing after forcing the button to show

Committed: https://crrev.com/b73810563987686c2e4f22ba98a10fa0275372b0
Cr-Commit-Position: refs/heads/master@{#402595}

[James Cook, 2016-06-28 17:43:50]

Description was changed from

==========
mash: Convert system tray logout button to wm common types

* Fold LastWindowClosedLogoutReminder into LogoutConfirmationController
* Move LogoutConfirmationController ownership from Shell to WmShell
* Place the logout confirmation dialog in the system modal container, not
in the root (dialogs should not live directly in the root)
* Move to a non-chromeos directory, because there is nothing chromeos-specific
about this code

BUG=619636
TEST=ash_unittests, manual testing after forcing the button to show
==========

to

==========
mash: Convert system tray logout button to wm common types

* Fold LastWindowClosedLogoutReminder into LogoutConfirmationController
* Move LogoutConfirmationController ownership from Shell to WmShell
* Place the logout confirmation dialog in the system modal container, not
in the root (dialogs should not live directly in the root)
* Move tray logout button a non-chromeos directory, because there is nothing
chromeos-specific about this code
* Move LabelTrayView to //ash/common/system/tray since it is not chromeos-
specific either

BUG=619636
TEST=ash_unittests, manual testing after forcing the button to show
==========

[James Cook, 2016-06-28 17:45:30]

Description was changed from

==========
mash: Convert system tray logout button to wm common types

* Fold LastWindowClosedLogoutReminder into LogoutConfirmationController
* Move LogoutConfirmationController ownership from Shell to WmShell
* Place the logout confirmation dialog in the system modal container, not
in the root (dialogs should not live directly in the root)
* Move tray logout button a non-chromeos directory, because there is nothing
chromeos-specific about this code
* Move LabelTrayView to //ash/common/system/tray since it is not chromeos-
specific either

BUG=619636
TEST=ash_unittests, manual testing after forcing the button to show
==========

to

==========
mash: Convert system tray logout button to wm common types

* Fold LastWindowClosedLogoutReminder into LogoutConfirmationController
* Move LogoutConfirmationController ownership from Shell to WmShell
* Place the logout confirmation dialog in the system modal container, not
in the root (dialogs should not live directly in the root)
* Move LabelTrayView to //ash/common/system/tray since it is not chromeos-
specific

BUG=619636
TEST=ash_unittests, manual testing after forcing the button to show
==========

[James Cook, 2016-06-28 18:09:53]

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

[James Cook, 2016-06-28 18:09:54]

msw, please take a look.

[msw, 2016-06-28 19:31:04]

https://codereview.chromium.org/2108793002/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/session/logout_confirmation_dialog.cc (right):

https://codereview.chromium.org/2108793002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/session/logout_confirmation_dialog.cc:52:
WmShell::Get()
aside: I wish this were easier, but I don't see a better pattern offhand.

https://codereview.chromium.org/2108793002/diff/40001/ash/common/wm_shell.cc
File ash/common/wm_shell.cc (right):

https://codereview.chromium.org/2108793002/diff/40001/ash/common/wm_shell.cc#...
ash/common/wm_shell.cc:39: WmRootWindowController*
WmShell::GetPrimaryRootWindowController() {
I have concerns about making this easy for non-test code (similar to primary
shelf/tray/etc)... can callers just inline this operation? will there be many?

https://codereview.chromium.org/2108793002/diff/40001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2108793002/diff/40001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:188: // Must be created after SystemTrayDelegate.
nit: // Must be created after and destroyed before SystemTrayDelegate.

https://codereview.chromium.org/2108793002/diff/40001/ash/shell.cc
File ash/shell.cc (left):

https://codereview.chromium.org/2108793002/diff/40001/ash/shell.cc#oldcode708
ash/shell.cc:708: wm_shell_->system_tray_delegate()->Shutdown();
Nice, this is already part of WmShell::SetSystemTrayDelegate
(afaict, the impls are all no-ops, I wonder if we can remove it separately?)

https://codereview.chromium.org/2108793002/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2108793002/diff/40001/ash/shell.cc#newcode689
ash/shell.cc:689: wm_shell_->DeleteLogoutConfirmationController();
It's unfortunate that ash::Shell controls the lifetime order of objects in the
WmShell; hopefully we can clean this up when SystemTray moves... Or could the
Create/Delete just be part of SetSystemTrayDelegate (toggled by null or not?)

[James Cook, 2016-06-28 20:14:46]

msw, please take another look.

https://codereview.chromium.org/2108793002/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/session/logout_confirmation_dialog.cc (right):

https://codereview.chromium.org/2108793002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/session/logout_confirmation_dialog.cc:52:
WmShell::Get()
On 2016/06/28 19:31:03, msw wrote:
> aside: I wish this were easier, but I don't see a better pattern offhand.

Acknowledged.

https://codereview.chromium.org/2108793002/diff/40001/ash/common/wm_shell.cc
File ash/common/wm_shell.cc (right):

https://codereview.chromium.org/2108793002/diff/40001/ash/common/wm_shell.cc#...
ash/common/wm_shell.cc:39: WmRootWindowController*
WmShell::GetPrimaryRootWindowController() {
On 2016/06/28 19:31:03, msw wrote:
> I have concerns about making this easy for non-test code (similar to primary
> shelf/tray/etc)... can callers just inline this operation? will there be many?

Inlined it. There will be a few, but forcing them to inline probably provides
good pressure in the direction we want to go.

https://codereview.chromium.org/2108793002/diff/40001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2108793002/diff/40001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:188: // Must be created after SystemTrayDelegate.
On 2016/06/28 19:31:04, msw wrote:
> nit: // Must be created after and destroyed before SystemTrayDelegate.

Deleted these methods.

https://codereview.chromium.org/2108793002/diff/40001/ash/shell.cc
File ash/shell.cc (left):

https://codereview.chromium.org/2108793002/diff/40001/ash/shell.cc#oldcode708
ash/shell.cc:708: wm_shell_->system_tray_delegate()->Shutdown();
On 2016/06/28 19:31:04, msw wrote:
> Nice, this is already part of WmShell::SetSystemTrayDelegate
> (afaict, the impls are all no-ops, I wonder if we can remove it separately?)

Removed the Shutdown() methods.

https://codereview.chromium.org/2108793002/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2108793002/diff/40001/ash/shell.cc#newcode689
ash/shell.cc:689: wm_shell_->DeleteLogoutConfirmationController();
On 2016/06/28 19:31:04, msw wrote:
> It's unfortunate that ash::Shell controls the lifetime order of objects in the
> WmShell; hopefully we can clean this up when SystemTray moves... Or could the
> Create/Delete just be part of SetSystemTrayDelegate (toggled by null or not?)

I moved the creation/deletion into WmShell. I split SetSystemTrayDelegate() into
separate Set and Delete methods because it makes it more readable with the
chromeos ifdefs.

[msw, 2016-06-28 21:09:16]

lgtm with WmShell::GetPrimaryRootWindowController removal.

https://codereview.chromium.org/2108793002/diff/40001/ash/common/wm_shell.cc
File ash/common/wm_shell.cc (right):

https://codereview.chromium.org/2108793002/diff/40001/ash/common/wm_shell.cc#...
ash/common/wm_shell.cc:39: WmRootWindowController*
WmShell::GetPrimaryRootWindowController() {
On 2016/06/28 20:14:46, James Cook wrote:
> On 2016/06/28 19:31:03, msw wrote:
> > I have concerns about making this easy for non-test code (similar to primary
> > shelf/tray/etc)... can callers just inline this operation? will there be
many?
> 
> Inlined it. There will be a few, but forcing them to inline probably provides
> good pressure in the direction we want to go.

Please actually remove this function; otherwise nice.

https://codereview.chromium.org/2108793002/diff/60001/ash/common/wm_shell.cc
File ash/common/wm_shell.cc (right):

https://codereview.chromium.org/2108793002/diff/60001/ash/common/wm_shell.cc#...
ash/common/wm_shell.cc:81: DCHECK(system_tray_delegate_);
optional nit: DCHECK at top?

[James Cook, 2016-06-28 21:41:02]

Thanks for the review!

https://codereview.chromium.org/2108793002/diff/40001/ash/common/wm_shell.cc
File ash/common/wm_shell.cc (right):

https://codereview.chromium.org/2108793002/diff/40001/ash/common/wm_shell.cc#...
ash/common/wm_shell.cc:39: WmRootWindowController*
WmShell::GetPrimaryRootWindowController() {
On 2016/06/28 21:09:16, msw wrote:
> On 2016/06/28 20:14:46, James Cook wrote:
> > On 2016/06/28 19:31:03, msw wrote:
> > > I have concerns about making this easy for non-test code (similar to
primary
> > > shelf/tray/etc)... can callers just inline this operation? will there be
> many?
> > 
> > Inlined it. There will be a few, but forcing them to inline probably
provides
> > good pressure in the direction we want to go.
> 
> Please actually remove this function; otherwise nice.

Doh, forgot to actually remove it! Done.

https://codereview.chromium.org/2108793002/diff/60001/ash/common/wm_shell.cc
File ash/common/wm_shell.cc (right):

https://codereview.chromium.org/2108793002/diff/60001/ash/common/wm_shell.cc#...
ash/common/wm_shell.cc:81: DCHECK(system_tray_delegate_);
On 2016/06/28 21:09:16, msw wrote:
> optional nit: DCHECK at top?

Done.

[James Cook, 2016-06-28 21:42:37]

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

[James Cook, 2016-06-28 21:42:38]

sky, can I get OWNERS for ui/views/window/* ?

[sky, 2016-06-28 22:14:41]

LGTM

[James Cook, 2016-06-28 22:25:03]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-06-28 22:25:04]

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/2108793002/#ps80001
(title: "more review comments, fix mash_unittests")

[commit-bot: I haz the power, 2016-06-28 22:26:47]

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, 2016-06-28 23:44:43]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-28 23:47:23]

Description was changed from

==========
mash: Convert system tray logout button to wm common types

* Fold LastWindowClosedLogoutReminder into LogoutConfirmationController
* Move LogoutConfirmationController ownership from Shell to WmShell
* Place the logout confirmation dialog in the system modal container, not
in the root (dialogs should not live directly in the root)
* Move LabelTrayView to //ash/common/system/tray since it is not chromeos-
specific

BUG=619636
TEST=ash_unittests, manual testing after forcing the button to show
==========

to

==========
mash: Convert system tray logout button to wm common types

* Fold LastWindowClosedLogoutReminder into LogoutConfirmationController
* Move LogoutConfirmationController ownership from Shell to WmShell
* Place the logout confirmation dialog in the system modal container, not
in the root (dialogs should not live directly in the root)
* Move LabelTrayView to //ash/common/system/tray since it is not chromeos-
specific

BUG=619636
TEST=ash_unittests, manual testing after forcing the button to show

Committed: https://crrev.com/b73810563987686c2e4f22ba98a10fa0275372b0
Cr-Commit-Position: refs/heads/master@{#402595}
==========

[commit-bot: I haz the power, 2016-06-28 23:47:25]

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