Connector.StartService is poorly named and misused as a result |
|
Issue description
It should be called something like WarmService or EnsureMatchingServiceInstanceRunning or something like that. I think the intent should always be limited to pre-rolling service instance startup when a client knows it's going to need to use it soon.
Currently it gets used to mean "I expect this service to not be running now, and I want to make it run so that it does something on startup."
Services should generally not do interesting things on startup though (at least not things which are interesting from any client's perspective), because most clients can't and shouldn't be able to reason about when service instances are going to be started. Trying to elicit meaningful side effects by invoking StartService() -- beyond the intent of "make sure this is running ASAP because I'm gonna use its interfaces" -- is likely to eventually lead to confusing or incorrect behavior.
It's also problematic because in general services should kill themselves when they realize they're idle. For most services, "idle" means that the last known client has closed the last interface pipe to the service. If no client ever acquires a pipe, there's no chance to observe that event. So if StartService were inadvertently used with a service which controls its lifecycle this way, we'd end up with stale leaky processes.
Fortunately it's only used today in cases where either someone is going to imminently bind an interface (like with Network Service) or with UI-facing services whose lifetime is controlled by user actions alone.
In the latter cases, we should instead add an explicit interface to elicit the desired behavior. We could for example define a common interface like:
module ui.mojom;
interface UserApplication {
// Ensures that any toplevel UI elements (e.g. main window) are created
// for a UI-driven service. Once this is called, the service instance
// may be expected to run until explicitly terminated through user action.
EnsureUiVisible();
};
And have things like quick_launch and tap_visualizer implement this rather than showing their UI in Service.OnStart() or whatever. Then when we want those services to show UI, we can do:
ui::mojom::UserApplicationPtr application;
connector->BindInterface(
service_manager::ServiceFilter::ByName(quick_launch::mojom::kServiceName),
mojo::MakeRequest(&application));
application->EnsureUiVisible();
This is obviously more verbose than connector->StartService(...) but it's also more technically correct and therefore less error-prone and confusing in the limit.
Note that mash defines Launchable which is quite similar in spirit, but I would expect us to define something more generic at the //ui layer.
Once the aforementioned interface is defined and used in place of StartService where appropriate, we can justify renaming StartService to WarmService or something similar, and avoid this confusion moving forward.
,
Jan 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1d323b303e9dffc236aff587691a6aa156193ad1 commit 1d323b303e9dffc236aff587691a6aa156193ad1 Author: James Cook <jamescook@chromium.org> Date: Wed Jan 02 19:42:29 2019 chromeos: Don't use WarmService to start tap_visualizer Instead define an explicit Show() method to start the UI. See bug for details. TBR=sky@chromium.org Bug: 904148 Test: Run chrome --show-taps, see touch point circles Change-Id: I9d718097be9bb685e1e218780489fffce386a622 Reviewed-on: https://chromium-review.googlesource.com/c/1385013 Reviewed-by: James Cook <jamescook@chromium.org> Reviewed-by: Ken Rockot <rockot@google.com> Commit-Queue: James Cook <jamescook@chromium.org> Cr-Commit-Position: refs/heads/master@{#619458} [modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/components/tap_visualizer/BUILD.gn [modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/components/tap_visualizer/manifest.json [modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/components/tap_visualizer/public/mojom/BUILD.gn [rename] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/components/tap_visualizer/public/mojom/tap_visualizer.mojom [modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/components/tap_visualizer/tap_visualizer_app.cc [modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/components/tap_visualizer/tap_visualizer_app.h [modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/components/tap_visualizer/tap_visualizer_app_unittest.cc [modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/shell.cc [modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/shell/ash_content_browser_manifest_overlay.json [modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/shell/content/client/shell_browser_main_parts.cc [modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/shell/content/client/shell_content_browser_client.cc [modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/shell/content/client/shell_main_delegate.cc [modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/chrome/browser/ash_service_registry.cc [modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/chrome/browser/chrome_content_browser_manifest_overlay.json [modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/chrome/utility/mash_service_factory.cc |
|
►
Sign in to add a comment |
|
Comment 1 by rockot@google.com
, Dec 3