Replace DBusThreadManager::GetFooClient with FooClient::Get() |
|||||
Issue descriptionSee long discussion of DBus code structure at go/chromeos-dbus-clients and background at go/dbus-in-mustash From hashimoto: DBusThreadManager was designed in a single-process world. If we were to design D-Bus code for mus+ash from scratch, I think we would use FooClient::Get() (i.e. each client class has a static Get() method) instead of DBusThreadManager::GetFooClient(). DBusThreadManager just manages the D-Bus thread and the Bus object. DBusThreadManager::GetFooClient() will be replaced with FooClient::Get(). During browser/ash initialization & shutdown, FooClient::Initialize() and FooClient::Shutdown() will be called. FooClient::Initialize() is responsible to interpret the commandline flag to switch fake/real impl. This way, developers don’t have to wonder which DBusThreadManager/ClientBundle to use, no need to link unnecessary client classes, and of course we can utilize DEPS check. This requires rewriting all DBusThreadManager user code, so in terms of workload it doesn’t look good. jamescook: There are ~40 unit tests that call DBusThreadManager::Initialize() and we will need to figure out what subset of clients need to be initialized for each one. Also, it would be nice if the --dbus-unstub-clients command line flag could be parsed once only. But overall, I like this. stevenjb: Generally I agree. The clients were only all lumped together as a convenient migration step from the "chromeos library" model. For unit tests we should really just be initializing the clients that the tests need. We can have convenience "packages" for multi-client systems like networking (which could be handled by NetworkHandler). So what I suggest is: * Change to FooClient::Initialize(), FooClient::InitializeForTesting() and FooClient::Shutdown(). * Create "bundles" of clients to group together initialization for each process (like DBusClientsBrowser, DBusClientsAsh and DBusClientsCommon). * Maybe create smaller bundles for things like networking. * Use //ash/DEPS to limit ash to just the clients it can use, and //chrome/browser/chromeos/DEPS for the browser. I'll do some of the initial work here around "bundles", since I need that to support skipping the initialization of browser-only dbus clients in the ash process.
,
Sep 19 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9be05a4b2ac43b54230f6c27711882176c377788 commit 9be05a4b2ac43b54230f6c27711882176c377788 Author: jamescook <jamescook@chromium.org> Date: Mon Sep 19 19:09:49 2016 chromeos; Remove unused switch --dbus-unstub-clients / --dbus-real-clients These are unused in the chromium and chromiumos source trees and no one on the mailing list is actually using them. Remove them significantly simplifies D-Bus client initialization -- only the D-Bus thread creation needs to stay centralized. * Remove DBusClientTypes and the string representations of them * Delete the parser for the switch * Initialize the client bundles as either all real or all fake * Remove a bunch of Init/Create methods from DBusThreadManager since there are fewer conditional cases BUG=647367, 644414 TEST=chromeos_unittests, ash_unittests, unit_tests TBR=tbarzic@chromium.org for easy_unlock Review-Url: https://codereview.chromium.org/2350543002 Cr-Commit-Position: refs/heads/master@{#419524} [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/ash/mus/window_manager_application.cc [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/ash/test/ash_test_helper.cc [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chrome/browser/chromeos/chrome_browser_main_chromeos.cc [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chrome/browser/chromeos/login/chrome_restart_request.cc [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chrome/test/base/testing_io_thread_state.cc [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chrome/test/base/view_event_test_platform_part_chromeos.cc [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chromeos/chromeos.gyp [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chromeos/chromeos_switches.cc [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chromeos/chromeos_switches.h [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chromeos/dbus/cros_disks_client.h [delete] https://crrev.com/8406717810569b784d50d9a907ac306586b5f6aa/chromeos/dbus/dbus_client_bundle_unittest.cc [add] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chromeos/dbus/dbus_client_implementation_type.h [delete] https://crrev.com/8406717810569b784d50d9a907ac306586b5f6aa/chromeos/dbus/dbus_client_types.cc [delete] https://crrev.com/8406717810569b784d50d9a907ac306586b5f6aa/chromeos/dbus/dbus_client_types.h [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chromeos/dbus/dbus_clients_browser.cc [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chromeos/dbus/dbus_clients_browser.h [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chromeos/dbus/dbus_clients_common.cc [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chromeos/dbus/dbus_clients_common.h [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chromeos/dbus/dbus_thread_manager.cc [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chromeos/dbus/dbus_thread_manager.h [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chromeos/dbus/dbus_thread_manager_unittest.cc [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chromeos/dbus/power_manager_client.h [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chromeos/dbus/session_manager_client.h [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/chromeos/dbus/update_engine_client.h [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/content/shell/browser/shell_browser_main_parts.cc [modify] https://crrev.com/9be05a4b2ac43b54230f6c27711882176c377788/extensions/shell/browser/shell_browser_main_parts.cc
,
Sep 27 2016
On hold for a bit to work on ash <-> chrome communication. Will pick up again soon when working on networking UI.
,
Jan 13 2017
Do we still think we need to do this? Definitely not needed for Q1.
,
Apr 19 2018
,
Aug 10
Steven, can you triage this bug? |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by bugdroid1@chromium.org
, Sep 19 2016