chromeos: Refactor D-Bus client creation for ash and browser processes

chromeos: Refactor D-Bus client creation for ash and browser processes

In mustash we need to have some D-Bus clients in the ash window manager
process and some in the browser process. In traditional ash we need everything
in the browser process. Allow per-process initialization of subsets of clients.

* Split ownership of clients into DBusClientsBrowser, DBusClientsAsh and
DBusClientsCommon
* Make DBusThreadManager::Initialize() take a process enum. This isn't great,
but see code review comments
* Remove unnecessary setters from DBusThreadManagerSetter
* Use ash/DEPS to restrict which clients can be used in //ash
* Only initialize ash clients in ash_unittests
* When running in mustash, limit which clients are initialized in the ash
and browser processes

This is a transitional step toward cleaner multi-process initialization of
clients as discussed in go/chromeos-dbus-clients option (G)

BUG=644414, 647367
TEST=chromeos_unittests, ash_unittests

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

[James Cook, 2016-09-15 21:52:13]

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

[commit-bot: I haz the power, 2016-09-15 21:52:53]

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

[James Cook, 2016-09-15 22:02:41]

Description was changed from

==========
WIP: chromeos: Refactor D-Bus client creation for ash and browser processes

In mustash we need to have some D-Bus clients in the ash window manager
process and some in the browser process. In traditional ash we need everything
in the browser process. Allow per-process initialization of subsets of clients.

This is a step toward cleaner multi-process initialization of clients as
discussed in go/chromeos-dbus-clients option (G)

BUG=
==========

to

==========
chromeos: Refactor D-Bus client creation for ash and browser processes

In mustash we need to have some D-Bus clients in the ash window manager
process and some in the browser process. In traditional ash we need everything
in the browser process. Allow per-process initialization of subsets of clients.

* Split ownership of clients into DBusClientsBrowser, DBusClientsAsh and
DBusClientsCommon
* Make DBusThreadManager::Initialize() take a process enum. This isn't great,
but see code review comments
* Remove unnecessary setters from DBusThreadManagerSetter
* Use ash/DEPS to restrict which clients can be used in //ash
* Only initialize ash clients in ash_unittests
* When running in mustash, limit which clients are initialized in the ash
and browser processes

This is a step toward cleaner multi-process initialization of clients as
discussed in go/chromeos-dbus-clients option (G)

BUG=644414,647367
TEST=chromeos_unittests, ash_unittests
==========

[James Cook, 2016-09-15 22:12:57]

Description was changed from

==========
chromeos: Refactor D-Bus client creation for ash and browser processes

In mustash we need to have some D-Bus clients in the ash window manager
process and some in the browser process. In traditional ash we need everything
in the browser process. Allow per-process initialization of subsets of clients.

* Split ownership of clients into DBusClientsBrowser, DBusClientsAsh and
DBusClientsCommon
* Make DBusThreadManager::Initialize() take a process enum. This isn't great,
but see code review comments
* Remove unnecessary setters from DBusThreadManagerSetter
* Use ash/DEPS to restrict which clients can be used in //ash
* Only initialize ash clients in ash_unittests
* When running in mustash, limit which clients are initialized in the ash
and browser processes

This is a step toward cleaner multi-process initialization of clients as
discussed in go/chromeos-dbus-clients option (G)

BUG=644414,647367
TEST=chromeos_unittests, ash_unittests
==========

to

==========
chromeos: Refactor D-Bus client creation for ash and browser processes

In mustash we need to have some D-Bus clients in the ash window manager
process and some in the browser process. In traditional ash we need everything
in the browser process. Allow per-process initialization of subsets of clients.

* Split ownership of clients into DBusClientsBrowser, DBusClientsAsh and
DBusClientsCommon
* Make DBusThreadManager::Initialize() take a process enum. This isn't great,
but see code review comments
* Remove unnecessary setters from DBusThreadManagerSetter
* Use ash/DEPS to restrict which clients can be used in //ash
* Only initialize ash clients in ash_unittests
* When running in mustash, limit which clients are initialized in the ash
and browser processes

This is a transitional step toward cleaner multi-process initialization of
clients as discussed in go/chromeos-dbus-clients option (G)

BUG=644414,647367
TEST=chromeos_unittests, ash_unittests
==========

[James Cook, 2016-09-15 22:23:09]

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

[James Cook, 2016-09-15 22:23:10]

Steven, please take a look.

This is a compromise first step. It does not move around files under
//chromeos/dbus. Moving a dbus client between processes just involves modifying
the DBusClients(Ash|Browser) classes and DEPS.

For now DBusThreadManager has knowledge of processes. We can eliminate that
knowledge if we can eliminate DBusThreadManager::GetFooClient().

https://codereview.chromium.org/2343993003/diff/20001/ash/DEPS
File ash/DEPS (right):

https://codereview.chromium.org/2343993003/diff/20001/ash/DEPS#newcode29
ash/DEPS:29: "+chromeos/dbus/session_manager_client.h",
Aside: I was surprised with how few of these were required. Something to think
about for the future is how to indicate which "manager" code should be
allowed/banned in ash. For example, ash should not be allowed to use
//chromeos/disks, at least for now. I plan to revisit DEPS in the future, so I
didn't change the DEPS for //chrome/browser/chromeos

https://codereview.chromium.org/2343993003/diff/20001/ash/test/ash_test_helpe...
File ash/test/ash_test_helper.cc (right):

https://codereview.chromium.org/2343993003/diff/20001/ash/test/ash_test_helpe...
ash/test/ash_test_helper.cc:96: chromeos::DBusThreadManager::Initialize(
By only initializing ash-process clients in ash tests we should be able to catch
future accidental usage, at least if the developer writes a test.

https://codereview.chromium.org/2343993003/diff/20001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_clients_ash.h (right):

https://codereview.chromium.org/2343993003/diff/20001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_clients_ash.h:26: class CHROMEOS_EXPORT DBusClientsAsh {
I intentionally do not provide DBusClientsAsh::Get() or accessors like
gsm_sms_client(). We could add that in another CL if we want. In the long term,
however, code should be calling FooClient::Get() to retrieve the client, so I
didn't want to encourage getting it from here.

https://codereview.chromium.org/2343993003/diff/20001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager_unittest.cc (right):

https://codereview.chromium.org/2343993003/diff/20001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager_unittest.cc:68: // Tests that clients can be
created for the browser process.
These tests give a nice summary of which client moved where.

[stevenjb, 2016-09-16 15:18:00]

This seems like a very good compromise, thanks James for taking the time to
gather feedback and put this together.

LGTM!

https://codereview.chromium.org/2343993003/diff/20001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_clients_ash.h (right):

https://codereview.chromium.org/2343993003/diff/20001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_clients_ash.h:26: class CHROMEOS_EXPORT DBusClientsAsh {
On 2016/09/15 22:23:10, James Cook wrote:
> I intentionally do not provide DBusClientsAsh::Get() or accessors like
> gsm_sms_client(). We could add that in another CL if we want. In the long
term,
> however, code should be calling FooClient::Get() to retrieve the client, so I
> didn't want to encourage getting it from here.

Agreed.

[James Cook, 2016-09-16 16:21:01]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-09-16 16:21:01]

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

[commit-bot: I haz the power, 2016-09-16 16:21:18]

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-09-16 17:09:00]

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

[commit-bot: I haz the power, 2016-09-16 17:09:01]

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_...)

[James Cook, 2016-09-16 23:28:34]

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

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

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

[James Cook, 2016-09-16 23:36:11]

Steven, please take another look.

I had to move the SMS stuff back into the common clients bundle. I got bitten by
a dependency problem. Even though NetworkSmsHandler is only actually used in
//ash, it gets touched during startup via //chrome/browser. I introduced a
browser startup crash (only on mustash) like this:

NetworkHandler::Init()
NetworkSmsHandler::Init()
DBusThreadManager::Get()->GetShillManagerClient()->GetProperties()
FakeShillDeviceClient::PassStubDeviceProperties()
NetworkSmsHandler::DevicePropertiesCallback()
new ModemManager1NetworkSmsDeviceHandler
DBusThreadManager::Get()->GetModemMessagingClient()

(Actual stack is really ugly due to callbacks, see
https://paste.googleplex.com/4673140206600192 )

This can be fixed by breaking the NetworkHandler / NetworkSmsHandler dependency,
but that's not trivial due to the call to GetShillManagerClient(). That refactor
is out-of-scope for this CL, so I just put the SMS stuff back in the same bundle
as shill. That left the ash clients bundle empty, so I removed it.

[commit-bot: I haz the power, 2016-09-17 00:25:07]

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

[commit-bot: I haz the power, 2016-09-17 00:25:08]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-09-19 16:05:24]

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

[commit-bot: I haz the power, 2016-09-19 16:05:38]

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

[stevenjb, 2016-09-19 16:22:45]

On 2016/09/16 23:36:11, James Cook wrote:
> Steven, please take another look.
> 
> I had to move the SMS stuff back into the common clients bundle. I got bitten
by
> a dependency problem. Even though NetworkSmsHandler is only actually used in
> //ash, it gets touched during startup via //chrome/browser. I introduced a
> browser startup crash (only on mustash) like this:
> 
> NetworkHandler::Init()
> NetworkSmsHandler::Init()
> DBusThreadManager::Get()->GetShillManagerClient()->GetProperties()
> FakeShillDeviceClient::PassStubDeviceProperties()
> NetworkSmsHandler::DevicePropertiesCallback()
> new ModemManager1NetworkSmsDeviceHandler
> DBusThreadManager::Get()->GetModemMessagingClient()
> 
> (Actual stack is really ugly due to callbacks, see
> https://paste.googleplex.com/4673140206600192 )
> 
> This can be fixed by breaking the NetworkHandler / NetworkSmsHandler
dependency,
> but that's not trivial due to the call to GetShillManagerClient(). That
refactor
> is out-of-scope for this CL, so I just put the SMS stuff back in the same
bundle
> as shill. That left the ash clients bundle empty, so I removed it.

Ugh, if SMS handler depends on ShillManagerClient then we can't really break it
out; I didn't realize that was the case.

[stevenjb, 2016-09-19 16:24:47]

LGTM

Let's plan to prioritize moving SMS to the message center instead, then we can
have the SMS logic in the same place as other network notification logic and
move them all together (or not).

[James Cook, 2016-09-19 16:36:40]

The CQ bit was unchecked by jamescook@chromium.org

[James Cook, 2016-09-19 16:36:47]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2016-09-19 16:36:59]

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-09-19 16:42:29]

Description was changed from

==========
chromeos: Refactor D-Bus client creation for ash and browser processes

In mustash we need to have some D-Bus clients in the ash window manager
process and some in the browser process. In traditional ash we need everything
in the browser process. Allow per-process initialization of subsets of clients.

* Split ownership of clients into DBusClientsBrowser, DBusClientsAsh and
DBusClientsCommon
* Make DBusThreadManager::Initialize() take a process enum. This isn't great,
but see code review comments
* Remove unnecessary setters from DBusThreadManagerSetter
* Use ash/DEPS to restrict which clients can be used in //ash
* Only initialize ash clients in ash_unittests
* When running in mustash, limit which clients are initialized in the ash
and browser processes

This is a transitional step toward cleaner multi-process initialization of
clients as discussed in go/chromeos-dbus-clients option (G)

BUG=644414,647367
TEST=chromeos_unittests, ash_unittests
==========

to

==========
chromeos: Refactor D-Bus client creation for ash and browser processes

In mustash we need to have some D-Bus clients in the ash window manager
process and some in the browser process. In traditional ash we need everything
in the browser process. Allow per-process initialization of subsets of clients.

* Split ownership of clients into DBusClientsBrowser, DBusClientsAsh and
DBusClientsCommon
* Make DBusThreadManager::Initialize() take a process enum. This isn't great,
but see code review comments
* Remove unnecessary setters from DBusThreadManagerSetter
* Use ash/DEPS to restrict which clients can be used in //ash
* Only initialize ash clients in ash_unittests
* When running in mustash, limit which clients are initialized in the ash
and browser processes

This is a transitional step toward cleaner multi-process initialization of
clients as discussed in go/chromeos-dbus-clients option (G)

BUG=644414,647367
TEST=chromeos_unittests, ash_unittests
==========

[commit-bot: I haz the power, 2016-09-19 16:42:30]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-19 16:47:01]

Description was changed from

==========
chromeos: Refactor D-Bus client creation for ash and browser processes

In mustash we need to have some D-Bus clients in the ash window manager
process and some in the browser process. In traditional ash we need everything
in the browser process. Allow per-process initialization of subsets of clients.

* Split ownership of clients into DBusClientsBrowser, DBusClientsAsh and
DBusClientsCommon
* Make DBusThreadManager::Initialize() take a process enum. This isn't great,
but see code review comments
* Remove unnecessary setters from DBusThreadManagerSetter
* Use ash/DEPS to restrict which clients can be used in //ash
* Only initialize ash clients in ash_unittests
* When running in mustash, limit which clients are initialized in the ash
and browser processes

This is a transitional step toward cleaner multi-process initialization of
clients as discussed in go/chromeos-dbus-clients option (G)

BUG=644414,647367
TEST=chromeos_unittests, ash_unittests
==========

to

==========
chromeos: Refactor D-Bus client creation for ash and browser processes

In mustash we need to have some D-Bus clients in the ash window manager
process and some in the browser process. In traditional ash we need everything
in the browser process. Allow per-process initialization of subsets of clients.

* Split ownership of clients into DBusClientsBrowser, DBusClientsAsh and
DBusClientsCommon
* Make DBusThreadManager::Initialize() take a process enum. This isn't great,
but see code review comments
* Remove unnecessary setters from DBusThreadManagerSetter
* Use ash/DEPS to restrict which clients can be used in //ash
* Only initialize ash clients in ash_unittests
* When running in mustash, limit which clients are initialized in the ash
and browser processes

This is a transitional step toward cleaner multi-process initialization of
clients as discussed in go/chromeos-dbus-clients option (G)

BUG=644414,647367
TEST=chromeos_unittests, ash_unittests

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

[commit-bot: I haz the power, 2016-09-19 16:47:02]

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