mash: remove the LockStateControllerDelegate.

mash: remove the LockStateControllerDelegate.

This removes the LockStateControllerDelegate and mojoifys/renames it
into the ShutdownClient, which is exported by chrome.

While in the long term, we'll want to move the code that's currently in
ShutdownClient into ash (and either use CrosSettings in ash or send
messages to a dedicated CrosSettings service), the chromeos device
settings code is sufficiently intertangled with the browser process that
it isn't happening anytime soon. (See crbug.com/446937)

BUG=628792
TEST=covered by converted ash_unittests; manually tested the mojo call in non-mash ash.

Committed: https://crrev.com/8f7f2a6d3baca46a3271e038d574f9187f08fa52
Cr-Commit-Position: refs/heads/master@{#429471}

[Elliot Glaysher, 2016-11-01 20:57:06]

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

[commit-bot: I haz the power, 2016-11-01 20:57:35]

Dry run: 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-11-01 21:15:08]

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

[commit-bot: I haz the power, 2016-11-01 21:15:08]

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

[Elliot Glaysher, 2016-11-01 22:02:45]

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

[commit-bot: I haz the power, 2016-11-01 22:03:31]

Dry run: 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-11-01 22:08:50]

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

[commit-bot: I haz the power, 2016-11-01 22:08:51]

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

[Elliot Glaysher, 2016-11-01 22:12:23]

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

[commit-bot: I haz the power, 2016-11-01 22:13:17]

Dry run: 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-11-01 22:18:55]

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

[commit-bot: I haz the power, 2016-11-01 22:18:57]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elliot Glaysher, 2016-11-01 22:21:35]

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

[commit-bot: I haz the power, 2016-11-01 22:22:36]

Dry run: 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-11-01 22:49:48]

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

[commit-bot: I haz the power, 2016-11-01 22:49:49]

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

[Elliot Glaysher, 2016-11-01 22:50:29]

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

[commit-bot: I haz the power, 2016-11-01 22:51:20]

Dry run: 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-11-01 22:58:41]

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

[commit-bot: I haz the power, 2016-11-01 22:58:42]

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

[Elliot Glaysher, 2016-11-01 23:08:29]

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

[commit-bot: I haz the power, 2016-11-01 23:09:07]

Dry run: 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-11-02 00:05:12]

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

[commit-bot: I haz the power, 2016-11-02 00:05:13]

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

[Elliot Glaysher, 2016-11-02 17:49:25]

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

[commit-bot: I haz the power, 2016-11-02 17:49:51]

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

[Elliot Glaysher, 2016-11-02 18:20:04]

Description was changed from

==========
mash: remove the LockStateControllerDelegate.

This removes the LockStateControllerDelegate and turns it into the
ShutdownClient, which is exported by chrome.

BUG=628792
==========

to

==========
mash: remove the LockStateControllerDelegate.

This removes the LockStateControllerDelegate and mojoifys/renames it
into the ShutdownClient, which is exported by chrome.

While in the long term, we'll want to move the code that's currently in
ShutdownClient into ash (and either use CrosSettings in ash or send
messages to a dedicated CrosSettings service), the chromeos device
settings code is sufficiently intertangled with the browser process that
it isn't happening anytime soon. (See crbug.com/446937)

BUG=628792
==========

[commit-bot: I haz the power, 2016-11-02 19:10:19]

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

[commit-bot: I haz the power, 2016-11-02 19:10:20]

Dry run: This issue passed the CQ dry run.

[Elliot Glaysher, 2016-11-02 19:13:27]

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

[Elliot Glaysher, 2016-11-02 19:13:29]

https://codereview.chromium.org/2471643002/diff/100001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2471643002/diff/100001/ash/shell.cc#newcode728
ash/shell.cc:728: wm_shell_->delegate()->GetShellConnector());
Q: LockStateController can't trivially be moved to WmShell because it requires
the root window to have been created during initialization. However, this
appears to be the first use of mojo in this file. Is this ok, or do I have to
add a post-window init phase to WmShell?

[Elliot Glaysher, 2016-11-02 20:06:21]

Description was changed from

==========
mash: remove the LockStateControllerDelegate.

This removes the LockStateControllerDelegate and mojoifys/renames it
into the ShutdownClient, which is exported by chrome.

While in the long term, we'll want to move the code that's currently in
ShutdownClient into ash (and either use CrosSettings in ash or send
messages to a dedicated CrosSettings service), the chromeos device
settings code is sufficiently intertangled with the browser process that
it isn't happening anytime soon. (See crbug.com/446937)

BUG=628792
==========

to

==========
mash: remove the LockStateControllerDelegate.

This removes the LockStateControllerDelegate and mojoifys/renames it
into the ShutdownClient, which is exported by chrome.

While in the long term, we'll want to move the code that's currently in
ShutdownClient into ash (and either use CrosSettings in ash or send
messages to a dedicated CrosSettings service), the chromeos device
settings code is sufficiently intertangled with the browser process that
it isn't happening anytime soon. (See crbug.com/446937)

BUG=628792
TEST=covered by converted ash_unittests; manually tested the mojo call in
non-mash ash.
==========

[James Cook, 2016-11-02 20:20:04]

Little stuff. I pondering making Chrome register itself during startup as the
ShutdownClient, but I think that's overkill.

https://codereview.chromium.org/2471643002/diff/100001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2471643002/diff/100001/ash/shell.cc#newcode728
ash/shell.cc:728: wm_shell_->delegate()->GetShellConnector());
On 2016/11/02 19:13:29, Elliot Glaysher wrote:
> Q: LockStateController can't trivially be moved to WmShell because it requires
> the root window to have been created during initialization. However, this
> appears to be the first use of mojo in this file. Is this ok, or do I have to
> add a post-window init phase to WmShell?

I think this is OK. WmShell's constructor touches the shell connector and is
created before this. Also, it looks like you're just creating a couple objects
and caching the connector pointer, which should be fine.

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/lock_state_cont...
File ash/wm/lock_state_controller.cc (right):

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/lock_state_cont...
ash/wm/lock_state_controller.cc:332: // Connect to the browser to tell it to
shutdown the computer. It will either
Thanks for the detailed docs and TODO.

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/lock_state_cont...
File ash/wm/lock_state_controller.h (right):

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/lock_state_cont...
ash/wm/lock_state_controller.h:12: #include
"ash/public/interfaces/shutdown.mojom.h"
Can this be forward declared?

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/lock_state_cont...
ash/wm/lock_state_controller.h:89: class ASH_EXPORT TestApi {
What do you think about moving the test API to a separate class/file? (It
wouldn't need to be in this CL.)

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/lock_state_cont...
ash/wm/lock_state_controller.h:137:
LockStateController(service_manager::Connector* connector);
nit: explicit

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/lock_state_cont...
File ash/wm/lock_state_controller_unittest.cc (right):

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/lock_state_cont...
ash/wm/lock_state_controller_unittest.cc:64: shutdown_client_ = new
TestShutdownClient);
nit: This construct made me do a double-take. Please split this into two
separate statements.

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/shutdown_client...
File ash/wm/shutdown_client_proxy.cc (right):

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/shutdown_client...
ash/wm/shutdown_client_proxy.cc:13: : connector_(connector) {}
nit: DCHECK connector_ (makes the parameter nullability self-documenting)

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/shutdown_client...
File ash/wm/shutdown_client_proxy.h (right):

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/shutdown_client...
ash/wm/shutdown_client_proxy.h:22:
ShutdownClientProxy(service_manager::Connector* connector);
nit: explicit

https://codereview.chromium.org/2471643002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/power/shutdown_client_impl.h (right):

https://codereview.chromium.org/2471643002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/power/shutdown_client_impl.h:9: #include
"base/compiler_specific.h"
Not needed?

[Elliot Glaysher, 2016-11-02 20:55:46]

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

[commit-bot: I haz the power, 2016-11-02 20:56:11]

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

[Elliot Glaysher, 2016-11-02 21:45:16]

ptal. I also moved the test api to its own file.

https://codereview.chromium.org/2471643002/diff/100001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2471643002/diff/100001/ash/shell.cc#newcode728
ash/shell.cc:728: wm_shell_->delegate()->GetShellConnector());
On 2016/11/02 20:20:03, James Cook wrote:
> On 2016/11/02 19:13:29, Elliot Glaysher wrote:
> > Q: LockStateController can't trivially be moved to WmShell because it
requires
> > the root window to have been created during initialization. However, this
> > appears to be the first use of mojo in this file. Is this ok, or do I have
to
> > add a post-window init phase to WmShell?
> 
> I think this is OK. WmShell's constructor touches the shell connector and is
> created before this. Also, it looks like you're just creating a couple objects
> and caching the connector pointer, which should be fine.

Acknowledged.

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/lock_state_cont...
File ash/wm/lock_state_controller.h (right):

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/lock_state_cont...
ash/wm/lock_state_controller.h:12: #include
"ash/public/interfaces/shutdown.mojom.h"
On 2016/11/02 20:20:03, James Cook wrote:
> Can this be forward declared?

Done.

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/lock_state_cont...
ash/wm/lock_state_controller.h:137:
LockStateController(service_manager::Connector* connector);
On 2016/11/02 20:20:03, James Cook wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/lock_state_cont...
File ash/wm/lock_state_controller_unittest.cc (right):

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/lock_state_cont...
ash/wm/lock_state_controller_unittest.cc:64: shutdown_client_ = new
TestShutdownClient);
On 2016/11/02 20:20:03, James Cook wrote:
> nit: This construct made me do a double-take. Please split this into two
> separate statements.

Done.

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/shutdown_client...
File ash/wm/shutdown_client_proxy.cc (right):

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/shutdown_client...
ash/wm/shutdown_client_proxy.cc:13: : connector_(connector) {}
On 2016/11/02 20:20:03, James Cook wrote:
> nit: DCHECK connector_ (makes the parameter nullability self-documenting)

Done.

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/shutdown_client...
File ash/wm/shutdown_client_proxy.h (right):

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/shutdown_client...
ash/wm/shutdown_client_proxy.h:22:
ShutdownClientProxy(service_manager::Connector* connector);
On 2016/11/02 20:20:04, James Cook wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2471643002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/power/shutdown_client_impl.h (right):

https://codereview.chromium.org/2471643002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/power/shutdown_client_impl.h:9: #include
"base/compiler_specific.h"
On 2016/11/02 20:20:04, James Cook wrote:
> Not needed?

You're right. Removed.

[James Cook, 2016-11-02 21:59:14]

LGTM

[commit-bot: I haz the power, 2016-11-02 22:11:26]

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

[commit-bot: I haz the power, 2016-11-02 22:11:27]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Elliot Glaysher, 2016-11-02 22:20:15]

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

[Elliot Glaysher, 2016-11-02 22:20:46]

erg@chromium.org changed reviewers:
+ sky@chromium.org, tsepez@chromium.org

[Elliot Glaysher, 2016-11-02 22:20:47]

sky: review
tsepez: mojom

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/shutdown_client...
File ash/wm/shutdown_client_proxy.cc (right):

https://codereview.chromium.org/2471643002/diff/100001/ash/wm/shutdown_client...
ash/wm/shutdown_client_proxy.cc:13: : connector_(connector) {}
On 2016/11/02 21:45:16, Elliot Glaysher wrote:
> On 2016/11/02 20:20:03, James Cook wrote:
> > nit: DCHECK connector_ (makes the parameter nullability self-documenting)
> 
> Done.

Reverted this one hunk. Breaks tests.

[commit-bot: I haz the power, 2016-11-02 22:21:12]

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

[Tom Sepez, 2016-11-02 22:27:22]

mojom LGTM

[sky, 2016-11-02 23:06:52]

https://codereview.chromium.org/2471643002/diff/140001/ash/test/test_shutdown...
File ash/test/test_shutdown_client.h (right):

https://codereview.chromium.org/2471643002/diff/140001/ash/test/test_shutdown...
ash/test/test_shutdown_client.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
2016

https://codereview.chromium.org/2471643002/diff/140001/ash/wm/lock_state_cont...
File ash/wm/lock_state_controller.cc (right):

https://codereview.chromium.org/2471643002/diff/140001/ash/wm/lock_state_cont...
ash/wm/lock_state_controller.cc:83: shutdown_client_(new
ShutdownClientProxy(connector)),
MakeUnique

https://codereview.chromium.org/2471643002/diff/140001/ash/wm/shutdown_client...
File ash/wm/shutdown_client_proxy.h (right):

https://codereview.chromium.org/2471643002/diff/140001/ash/wm/shutdown_client...
ash/wm/shutdown_client_proxy.h:19: // so we can mock this interface.
What do you mean by mocking? I don't see any subclasses of this class. Maybe I'm
misunderstanding this comment?

[Elliot Glaysher, 2016-11-02 23:10:30]

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

[commit-bot: I haz the power, 2016-11-02 23:11:07]

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

[Elliot Glaysher, 2016-11-02 23:12:28]

ptal

https://codereview.chromium.org/2471643002/diff/140001/ash/test/test_shutdown...
File ash/test/test_shutdown_client.h (right):

https://codereview.chromium.org/2471643002/diff/140001/ash/test/test_shutdown...
ash/test/test_shutdown_client.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
On 2016/11/02 23:06:52, sky wrote:
> 2016

Done.

https://codereview.chromium.org/2471643002/diff/140001/ash/wm/lock_state_cont...
File ash/wm/lock_state_controller.cc (right):

https://codereview.chromium.org/2471643002/diff/140001/ash/wm/lock_state_cont...
ash/wm/lock_state_controller.cc:83: shutdown_client_(new
ShutdownClientProxy(connector)),
On 2016/11/02 23:06:52, sky wrote:
> MakeUnique

Done.

https://codereview.chromium.org/2471643002/diff/140001/ash/wm/shutdown_client...
File ash/wm/shutdown_client_proxy.h (right):

https://codereview.chromium.org/2471643002/diff/140001/ash/wm/shutdown_client...
ash/wm/shutdown_client_proxy.h:19: // so we can mock this interface.
On 2016/11/02 23:06:52, sky wrote:
> What do you mean by mocking? I don't see any subclasses of this class. Maybe
I'm
> misunderstanding this comment?

Replaced comment with explicit reference to TestShutdownClient.

[sky, 2016-11-02 23:15:05]

Thanks for the clarification. I see the mocking is at the mojom::ShutdownClient,
which LGTM

[Elliot Glaysher, 2016-11-03 00:02:10]

The CQ bit was unchecked by erg@chromium.org

[Elliot Glaysher, 2016-11-03 00:02:16]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2016-11-03 00:02:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2471643002/#ps160001
(title: "sky comments")

[commit-bot: I haz the power, 2016-11-03 00:02:57]

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-11-03 00:21:07]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-11-03 00:22:42]

Description was changed from

==========
mash: remove the LockStateControllerDelegate.

This removes the LockStateControllerDelegate and mojoifys/renames it
into the ShutdownClient, which is exported by chrome.

While in the long term, we'll want to move the code that's currently in
ShutdownClient into ash (and either use CrosSettings in ash or send
messages to a dedicated CrosSettings service), the chromeos device
settings code is sufficiently intertangled with the browser process that
it isn't happening anytime soon. (See crbug.com/446937)

BUG=628792
TEST=covered by converted ash_unittests; manually tested the mojo call in
non-mash ash.
==========

to

==========
mash: remove the LockStateControllerDelegate.

This removes the LockStateControllerDelegate and mojoifys/renames it
into the ShutdownClient, which is exported by chrome.

While in the long term, we'll want to move the code that's currently in
ShutdownClient into ash (and either use CrosSettings in ash or send
messages to a dedicated CrosSettings service), the chromeos device
settings code is sufficiently intertangled with the browser process that
it isn't happening anytime soon. (See crbug.com/446937)

BUG=628792
TEST=covered by converted ash_unittests; manually tested the mojo call in
non-mash ash.

Committed: https://crrev.com/8f7f2a6d3baca46a3271e038d574f9187f08fa52
Cr-Commit-Position: refs/heads/master@{#429471}
==========

[commit-bot: I haz the power, 2016-11-03 00:22:44]

Patchset 9 (id:??) landed as
https://crrev.com/8f7f2a6d3baca46a3271e038d574f9187f08fa52
Cr-Commit-Position: refs/heads/master@{#429471}