mash: Make the power button shut the system down.

mash: Make the power button shut the system down.

Now that ash::Shell is instantiated in mash, let
LockStateController ask powerd to shut the system down when
requested. Also fix various crashes in LockStateController
due to CursorManager being uninitialized in mash and
remove WmShell::RequestShutdown() in favor of calling
LockStateController::RequestShutdown() directly.

BUG=679062, 632187
Review-Url: https://codereview.chromium.org/2652093002
Cr-Commit-Position: refs/heads/master@{#445962}
Committed: https://chromium.googlesource.com/chromium/src/+/c6250a0ced3c3b8d41214af299502af4dfefd850

[Daniel Erat, 2017-01-24 22:21:07]

The CQ bit was checked by derat@chromium.org to run a CQ dry run

[Daniel Erat, 2017-01-24 22:21:22]

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

[Daniel Erat, 2017-01-24 22:21:23]

https://codereview.chromium.org/2652093002/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2652093002/diff/1/ash/shell.cc#newcode682
ash/shell.cc:682: if (cursor_manager_) {
random style fix. let me know if you'd prefer that i remove this from the
change.

https://codereview.chromium.org/2652093002/diff/1/ash/wm/lock_state_controlle...
File ash/wm/lock_state_controller.cc (right):

https://codereview.chromium.org/2652093002/diff/1/ash/wm/lock_state_controlle...
ash/wm/lock_state_controller.cc:165: if (shell->cursor_manager()) {
ash::Shell initializes this only if !is_mash and has similar null checks
sprinkled throughout, but let me know if a different approach would be better.

[commit-bot: I haz the power, 2017-01-24 22:21:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Daniel Erat, 2017-01-24 22:24:33]

Description was changed from

==========
mash: Make the power button shut the system down.

Now that ash::Shell is instantiated in mash, let
LockStateController ask powerd to shut the system down when
requested. Also fix various crashes in LockStateController
due to CursorManager being uninitialized in mash.

BUG=679062
==========

to

==========
mash: Make the power button shut the system down.

Now that ash::Shell is instantiated in mash, let
LockStateController ask powerd to shut the system down when
requested. Also fix various crashes in LockStateController
due to CursorManager being uninitialized in mash.

BUG=679062,632187
==========

[sky, 2017-01-24 23:01:51]

LGTM

[commit-bot: I haz the power, 2017-01-25 00:21:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 00:21:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Daniel Erat, 2017-01-25 00:29:37]

The CQ bit was checked by derat@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 00:30:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2017-01-25 01:02:59]

https://codereview.chromium.org/2652093002/diff/20001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2652093002/diff/20001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:398:
Shell::GetInstance()->lock_state_controller()->RequestShutdown();
It looks like this is the same implementation as WmShellAura. Can the
implementation move into the base class WmShell, or better yet move back into
ash::Shell?

[Daniel Erat, 2017-01-25 01:10:51]

https://codereview.chromium.org/2652093002/diff/20001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2652093002/diff/20001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:398:
Shell::GetInstance()->lock_state_controller()->RequestShutdown();
On 2017/01/25 01:02:59, James Cook wrote:
> It looks like this is the same implementation as WmShellAura. Can the
> implementation move into the base class WmShell, or better yet move back into
> ash::Shell?

good suggestion. i think that this method is only called from
ash/common/system/date/date_default_view.cc and
ash/common/system/tiles/tiles_default_view.cc, but both of those files seem very
dependent on WmShell (and don't depend on ash::Shell at all).

would you prefer that i update them to instead call
ash::Shell::GetInstance()->lock_state_controller()->RequestShutdown(), or that i
move this RequestShutdown method to WmShell? in other words, are we all-in on
using the ash::Shell singleton now?

[James Cook, 2017-01-25 01:17:38]

https://codereview.chromium.org/2652093002/diff/20001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2652093002/diff/20001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:398:
Shell::GetInstance()->lock_state_controller()->RequestShutdown();
On 2017/01/25 01:10:51, Daniel Erat wrote:
> On 2017/01/25 01:02:59, James Cook wrote:
> > It looks like this is the same implementation as WmShellAura. Can the
> > implementation move into the base class WmShell, or better yet move back
into
> > ash::Shell?
> 
> good suggestion. i think that this method is only called from
> ash/common/system/date/date_default_view.cc and
> ash/common/system/tiles/tiles_default_view.cc, but both of those files seem
very
> dependent on WmShell (and don't depend on ash::Shell at all).
> 
> would you prefer that i update them to instead call
> ash::Shell::GetInstance()->lock_state_controller()->RequestShutdown(), or that
i
> move this RequestShutdown method to WmShell? in other words, are we all-in on
> using the ash::Shell singleton now?

I think we're OK using ash::Shell::GetInstance() now, and I think we should try
to keep things that are the same in classic ash and mash in ash::Shell.

(Those system files date to the time when ash::Shell was disallowed.)

[commit-bot: I haz the power, 2017-01-25 01:19:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 01:19:35]

Dry run: This issue passed the CQ dry run.

[Daniel Erat, 2017-01-25 01:40:20]

https://codereview.chromium.org/2652093002/diff/20001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2652093002/diff/20001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:398:
Shell::GetInstance()->lock_state_controller()->RequestShutdown();
On 2017/01/25 01:17:38, James Cook wrote:
> On 2017/01/25 01:10:51, Daniel Erat wrote:
> > On 2017/01/25 01:02:59, James Cook wrote:
> > > It looks like this is the same implementation as WmShellAura. Can the
> > > implementation move into the base class WmShell, or better yet move back
> into
> > > ash::Shell?
> > 
> > good suggestion. i think that this method is only called from
> > ash/common/system/date/date_default_view.cc and
> > ash/common/system/tiles/tiles_default_view.cc, but both of those files seem
> very
> > dependent on WmShell (and don't depend on ash::Shell at all).
> > 
> > would you prefer that i update them to instead call
> > ash::Shell::GetInstance()->lock_state_controller()->RequestShutdown(), or
that
> i
> > move this RequestShutdown method to WmShell? in other words, are we all-in
on
> > using the ash::Shell singleton now?
> 
> I think we're OK using ash::Shell::GetInstance() now, and I think we should
try
> to keep things that are the same in classic ash and mash in ash::Shell.
> 
> (Those system files date to the time when ash::Shell was disallowed.)

ah, i see that there's a relevant todo in ash/common/wm_shell.h:

  // Displays the shutdown animation and requests a system shutdown or system
  // restart depending on the the state of the |RebootOnShutdown| device policy.
  // TODO(mash): Remove this method and call LockStateController directly when
  // it is available to code in ash/common.
  virtual void RequestShutdown() = 0;

LockStateController is in ash/wm, so i think that code under ash/common can't
depend on it. moving LSC to ash/common may not be straightforward: it depends on
ash/wm/session_state_animator.h, which depends on ash/wm/window_animations.h,
etc.

should i just move the RequestShutdown() method to ash::Shell for now?

[James Cook, 2017-01-25 01:45:23]

https://codereview.chromium.org/2652093002/diff/20001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2652093002/diff/20001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:398:
Shell::GetInstance()->lock_state_controller()->RequestShutdown();
On 2017/01/25 01:40:19, Daniel Erat wrote:
> On 2017/01/25 01:17:38, James Cook wrote:
> > On 2017/01/25 01:10:51, Daniel Erat wrote:
> > > On 2017/01/25 01:02:59, James Cook wrote:
> > > > It looks like this is the same implementation as WmShellAura. Can the
> > > > implementation move into the base class WmShell, or better yet move back
> > into
> > > > ash::Shell?
> > > 
> > > good suggestion. i think that this method is only called from
> > > ash/common/system/date/date_default_view.cc and
> > > ash/common/system/tiles/tiles_default_view.cc, but both of those files
seem
> > very
> > > dependent on WmShell (and don't depend on ash::Shell at all).
> > > 
> > > would you prefer that i update them to instead call
> > > ash::Shell::GetInstance()->lock_state_controller()->RequestShutdown(), or
> that
> > i
> > > move this RequestShutdown method to WmShell? in other words, are we all-in
> on
> > > using the ash::Shell singleton now?
> > 
> > I think we're OK using ash::Shell::GetInstance() now, and I think we should
> try
> > to keep things that are the same in classic ash and mash in ash::Shell.
> > 
> > (Those system files date to the time when ash::Shell was disallowed.)
> 
> ah, i see that there's a relevant todo in ash/common/wm_shell.h:
> 
>   // Displays the shutdown animation and requests a system shutdown or system
>   // restart depending on the the state of the |RebootOnShutdown| device
policy.
>   // TODO(mash): Remove this method and call LockStateController directly when
>   // it is available to code in ash/common.
>   virtual void RequestShutdown() = 0;
> 
> LockStateController is in ash/wm, so i think that code under ash/common can't
> depend on it. moving LSC to ash/common may not be straightforward: it depends
on
> ash/wm/session_state_animator.h, which depends on ash/wm/window_animations.h,
> etc.
> 
> should i just move the RequestShutdown() method to ash::Shell for now?

I wouldn't move LSC to ash/common - I think this comment is slightly out of
date.

I think code in ash/common can in general use code in ash/wm now. (I mean, LSC
is being used in classic ash and with this CL we're going to use it mash also,
so there's no point to a deps ban. The deps in ash/common/DEPS originally had
-ash but don't anymore. I'm not sure why sky didn't remove all the other +ash
references.)  Does checkdeps complain?

If the DEPS end up getting in the way I'm OK with putting it in ash::Shell.

[Daniel Erat, 2017-01-25 02:06:10]

https://codereview.chromium.org/2652093002/diff/20001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2652093002/diff/20001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:398:
Shell::GetInstance()->lock_state_controller()->RequestShutdown();
On 2017/01/25 01:45:23, James Cook wrote:
> On 2017/01/25 01:40:19, Daniel Erat wrote:
> > On 2017/01/25 01:17:38, James Cook wrote:
> > > On 2017/01/25 01:10:51, Daniel Erat wrote:
> > > > On 2017/01/25 01:02:59, James Cook wrote:
> > > > > It looks like this is the same implementation as WmShellAura. Can the
> > > > > implementation move into the base class WmShell, or better yet move
back
> > > into
> > > > > ash::Shell?
> > > > 
> > > > good suggestion. i think that this method is only called from
> > > > ash/common/system/date/date_default_view.cc and
> > > > ash/common/system/tiles/tiles_default_view.cc, but both of those files
> seem
> > > very
> > > > dependent on WmShell (and don't depend on ash::Shell at all).
> > > > 
> > > > would you prefer that i update them to instead call
> > > > ash::Shell::GetInstance()->lock_state_controller()->RequestShutdown(),
or
> > that
> > > i
> > > > move this RequestShutdown method to WmShell? in other words, are we
all-in
> > on
> > > > using the ash::Shell singleton now?
> > > 
> > > I think we're OK using ash::Shell::GetInstance() now, and I think we
should
> > try
> > > to keep things that are the same in classic ash and mash in ash::Shell.
> > > 
> > > (Those system files date to the time when ash::Shell was disallowed.)
> > 
> > ah, i see that there's a relevant todo in ash/common/wm_shell.h:
> > 
> >   // Displays the shutdown animation and requests a system shutdown or
system
> >   // restart depending on the the state of the |RebootOnShutdown| device
> policy.
> >   // TODO(mash): Remove this method and call LockStateController directly
when
> >   // it is available to code in ash/common.
> >   virtual void RequestShutdown() = 0;
> > 
> > LockStateController is in ash/wm, so i think that code under ash/common
can't
> > depend on it. moving LSC to ash/common may not be straightforward: it
depends
> on
> > ash/wm/session_state_animator.h, which depends on
ash/wm/window_animations.h,
> > etc.
> > 
> > should i just move the RequestShutdown() method to ash::Shell for now?
> 
> I wouldn't move LSC to ash/common - I think this comment is slightly out of
> date.
> 
> I think code in ash/common can in general use code in ash/wm now. (I mean, LSC
> is being used in classic ash and with this CL we're going to use it mash also,
> so there's no point to a deps ban. The deps in ash/common/DEPS originally had
> -ash but don't anymore. I'm not sure why sky didn't remove all the other +ash
> references.)  Does checkdeps complain?
> 
> If the DEPS end up getting in the way I'm OK with putting it in ash::Shell.

thanks, i've made this change and am testing the DEPS now (via a cq dry run,
because i'm not quite sure how to run checkdeps.py locally...).

[Daniel Erat, 2017-01-25 02:06:16]

The CQ bit was checked by derat@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 02:07:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Daniel Erat, 2017-01-25 02:07:21]

Description was changed from

==========
mash: Make the power button shut the system down.

Now that ash::Shell is instantiated in mash, let
LockStateController ask powerd to shut the system down when
requested. Also fix various crashes in LockStateController
due to CursorManager being uninitialized in mash.

BUG=679062,632187
==========

to

==========
mash: Make the power button shut the system down.

Now that ash::Shell is instantiated in mash, let
LockStateController ask powerd to shut the system down when
requested. Also fix various crashes in LockStateController
due to CursorManager being uninitialized in mash and
remove WmShell::RequestShutdown() in favor of calling
LockStateController::RequestShutdown() directly.

BUG=679062,632187
==========

[James Cook, 2017-01-25 02:40:26]

LGTM with optional nit

For future reference, this runs checkdeps:
buildtools/checkdeps/checkdeps.py

You can also give it a single directory to speed it up:
buildtools/checkdeps/checkdeps.py ash
buildtools/checkdeps/checkdeps.py chrome

https://codereview.chromium.org/2652093002/diff/40001/ash/wm/lock_state_contr...
File ash/wm/lock_state_controller.cc (right):

https://codereview.chromium.org/2652093002/diff/40001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller.cc:165: if (shell->cursor_manager()) {
optional: WDYT of calling !WmShell::Get()->IsRunningInMash() for these? It might
make it clearer that it's a mash vs. classic ash difference (i.e. under classic
ash you should always have a cursor_manager()). Alternately, maybe comment this
is a mash thing?

(Sorry I didn't notice this before.)

[commit-bot: I haz the power, 2017-01-25 02:54:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 02:54:13]

Dry run: This issue passed the CQ dry run.

[Daniel Erat, 2017-01-25 05:22:03]

On 2017/01/25 02:40:26, James Cook wrote:
> LGTM with optional nit
> 
> For future reference, this runs checkdeps:
> buildtools/checkdeps/checkdeps.py
> 
> You can also give it a single directory to speed it up:
> buildtools/checkdeps/checkdeps.py ash
> buildtools/checkdeps/checkdeps.py chrome

ah, thanks. i had run "git ls-files" in the main repo, which just gave me the
not-very-useful testing/scripts/checkdeps.py.

>
https://codereview.chromium.org/2652093002/diff/40001/ash/wm/lock_state_contr...
> File ash/wm/lock_state_controller.cc (right):
> 
>
https://codereview.chromium.org/2652093002/diff/40001/ash/wm/lock_state_contr...
> ash/wm/lock_state_controller.cc:165: if (shell->cursor_manager()) {
> optional: WDYT of calling !WmShell::Get()->IsRunningInMash() for these? It
might
> make it clearer that it's a mash vs. classic ash difference (i.e. under
classic
> ash you should always have a cursor_manager()). Alternately, maybe comment
this
> is a mash thing?
> 
> (Sorry I didn't notice this before.)

[Daniel Erat, 2017-01-25 05:22:21]

https://codereview.chromium.org/2652093002/diff/40001/ash/wm/lock_state_contr...
File ash/wm/lock_state_controller.cc (right):

https://codereview.chromium.org/2652093002/diff/40001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller.cc:165: if (shell->cursor_manager()) {
On 2017/01/25 02:40:26, James Cook wrote:
> optional: WDYT of calling !WmShell::Get()->IsRunningInMash() for these? It
might
> make it clearer that it's a mash vs. classic ash difference (i.e. under
classic
> ash you should always have a cursor_manager()). Alternately, maybe comment
this
> is a mash thing?
> 
> (Sorry I didn't notice this before.)

presumably we're going to have a ws-aware CursorManager at some point, right?
i'll go the comment route for now.

[Daniel Erat, 2017-01-25 05:22:47]

The CQ bit was checked by derat@chromium.org

[Daniel Erat, 2017-01-25 05:22:48]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2652093002/#ps60001
(title: "add comment about CursorManager")

[commit-bot: I haz the power, 2017-01-25 05:23:03]

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, 2017-01-25 05:51:59]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485321767792460,
"parent_rev": "26b5760aa847ce3dc92e714a134fb11c5c3548ce", "commit_rev":
"c6250a0ced3c3b8d41214af299502af4dfefd850"}

[commit-bot: I haz the power, 2017-01-25 05:52:30]

Description was changed from

==========
mash: Make the power button shut the system down.

Now that ash::Shell is instantiated in mash, let
LockStateController ask powerd to shut the system down when
requested. Also fix various crashes in LockStateController
due to CursorManager being uninitialized in mash and
remove WmShell::RequestShutdown() in favor of calling
LockStateController::RequestShutdown() directly.

BUG=679062,632187
==========

to

==========
mash: Make the power button shut the system down.

Now that ash::Shell is instantiated in mash, let
LockStateController ask powerd to shut the system down when
requested. Also fix various crashes in LockStateController
due to CursorManager being uninitialized in mash and
remove WmShell::RequestShutdown() in favor of calling
LockStateController::RequestShutdown() directly.

BUG=679062,632187

Review-Url: https://codereview.chromium.org/2652093002
Cr-Commit-Position: refs/heads/master@{#445962}
Committed:
https://chromium.googlesource.com/chromium/src/+/c6250a0ced3c3b8d41214af29950...
==========

[commit-bot: I haz the power, 2017-01-25 05:52:31]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c6250a0ced3c3b8d41214af29950...