Is the thinking still that this should be in multiple services instead of all being within ash? Do we really plan to make mus provide one or more services via D-Bus?

I'm blocking this on  issue 692246 , which is about making it possible to instantiate multiple D-Bus services.

It seemed like some of the display/power stuff would be best handled by mus-ws, but it can go wherever you think is best.

It would be really nice if mus-ws handled the frecon display release, so you could switch to VT2 after a browser crash or ash crash.

Display taking/releasing is actually handled by a different provider (ConsoleServiceProvider).

It seems like it'd be nice if we could put TakeDisplayOwnership, ReleaseDisplayOwnership, SetDisplayPower, and SetDisplaySoftwareDimming all in the same D-Bus service implemented by mus-ws (assuming that there are no objections to or issues around making that process speak D-Bus -- are there?).

One wrinkle is that SetDisplaySoftwareDimming is currently implemented by ash::ScreenDimmer, which uses ash::WindowDimmer and does tricky layer-stacking stuff. If there's an easy way to instead implement that in mus-ws, great, but I'm a bit skeptical. I see that it's also gained logic to use different opacities depending on whether the screen is locked or not, for instance.

I'd like to keep SetDisplayPower and SetDisplaySoftwareDimming in the same D-Bus service, as it feels like we're leaking implementation details otherwise.

My current thinking is that we should create a single D-Bus service in mus-ws for this:

name:      org.chromium.DisplayService
path:      /org/chromium/DisplayService
interface: org.chromium.DisplayServiceInterface

I'll probably need to add a delegate for the SetDisplaySoftwareDimming functionality for now; I'm assuming we'll need IPC to do it under mash.

 Issue 644323  has been merged into this issue.

I had a mild preference for not making musws depend on dbus but in this case, it's probably the right medium-term thing to do.

Is your preference that the D-Bus interface instead lives in mash and does things using the Mojo interface exposed by musws? I think that'd mean that we'd be adding methods to the Mojo interface that are currently called only by powerd and frecon via D-Bus (SetDisplayPower and SetDisplaySoftwareDimming, and TakeDisplayOwnership and ReleaseDisplayOwnership, respectively), but maybe that's where we want to be in the long term in a post-D-Bus world where Chrome OS daemons are all using Mojo for IPC.

My mild preference was on the presumption that the daemons would be communicating via mojo. (someday :-) I agree with your plan -- musws should use dbus to control the console.

@11, as frecon has to run early in the boot process, loading Chrome libraries there is out of the question. Even D-bus is a bit of a problem.

I think that Lann is taking a look at this. It may be somewhat sprawling, so feel free to reach out to me and others if you get stuck.

As far as I'm aware (I hope someone corrects me if I'm wrong), the plan is still to introduce a new D-Bus service as described in #7 and have it (eventually) live in mus-ws.

The first step is probably to make DisplayPowerServiceProvider and ConsoleServiceProvider export their methods on a new service named org.chromium.DisplayService while continuing to export them on org.chromium.LibCrosService. See the DBusServices c'tor in chrome/browser/chromeos/chrome_browser_main_chromeos.cc.

Then all of their clients can be updated to call the methods in DisplayService rather than the ones in LibCrosService. This will probably be very similar to the already-completed work tracked by  issue 692246  (see its blocking bugs in particular).

After that is done and the methods are dropped from LibCrosService, we can look into making DisplayService be owned by mus-ws instead of by Chrome when running in a mode where mus-ws is present.

When I filed this bug the plan was that ash and mus-ws would be different processes. Now we think they will be separate services running in the same process. Is that going to cause problems here?

I don't think so, and I think splitting up the D-Bus services is the right thing to do, but I'm a little worried about stuff like having multiple DBusThreadManagers in a process.

I was assuming that mus-ws would own org.chromium.DisplayService and that it'd make a mojo call to ash for software dimming (which I think is still the only part of this that doesn't live in the mus-ws service; see #5).

Is your concern that ash needs a DBusThreadManager so it can listen to D-Bus signals (but not to export any D-Bus services of its own)? ash and mus-ws will still each have their own message loop, right? I'm assuming that they can each have a DBusThreadManager (cc-ing some people who'd know), although I agree that it feels a bit strange to create multiple D-Bus connections in a single process.

Yeah, it was more a speculative concern about having multiple D-Bus connections to a single process. Ash does more than just observe signals -- it looks like it calls methods too:

https://cs.chromium.org/chromium/src/ash/shutdown_controller.cc?type=cs&q=f:ash+dbusthreadmanager&sq=package:chromium&l=37

Hopefully that's OK. Mojo call from mus-ws to ash for dimming seems fine to me.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/system_api/+/dcb8a96005b0e1d1c21cdda06c204de9089a4e0a

commit dcb8a96005b0e1d1c21cdda06c204de9089a4e0a
Author: Lann Martin <lannm@chromium.org>
Date: Tue Jul 25 18:35:58 2017

Add org.chromium.DisplayService

Migrating display-related methods from LibCrosService.

BUG= chromium:644319 
TEST=none

Change-Id: I5daf2ef72c0d2620bb80006bc25c8ffea24036c6
Reviewed-on: https://chromium-review.googlesource.com/581938
Commit-Ready: Lann Martin <lannm@chromium.org>
Tested-by: Lann Martin <lannm@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/dcb8a96005b0e1d1c21cdda06c204de9089a4e0a/dbus/service_constants.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7f6f2893a7119bc34d59af855bfc3af4124ccb33

commit 7f6f2893a7119bc34d59af855bfc3af4124ccb33
Author: Lann Martin <lannm@chromium.org>
Date: Wed Jul 26 15:35:00 2017

dbus: Move methods to new org.chromium.DisplayService

These display-related methods are being moved from org.chromium.LibCrosService
to their own service so that they can be moved outside the browser process for
mus+ash.

BUG= 644319 
TEST=Manual dbus-send test on LibCrosService and DisplayService

Change-Id: Iad0547ded0d81f8fc599f93fa90f1918c4f018d2
Reviewed-on: https://chromium-review.googlesource.com/583853
Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Lann Martin <lannm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489653}
[modify] https://crrev.com/7f6f2893a7119bc34d59af855bfc3af4124ccb33/DEPS
[modify] https://crrev.com/7f6f2893a7119bc34d59af855bfc3af4124ccb33/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
[modify] https://crrev.com/7f6f2893a7119bc34d59af855bfc3af4124ccb33/chrome/browser/chromeos/dbus/chrome_console_service_provider_delegate.h
[modify] https://crrev.com/7f6f2893a7119bc34d59af855bfc3af4124ccb33/chromeos/BUILD.gn
[modify] https://crrev.com/7f6f2893a7119bc34d59af855bfc3af4124ccb33/chromeos/dbus/services/console_service_provider.cc
[modify] https://crrev.com/7f6f2893a7119bc34d59af855bfc3af4124ccb33/chromeos/dbus/services/console_service_provider.h
[modify] https://crrev.com/7f6f2893a7119bc34d59af855bfc3af4124ccb33/chromeos/dbus/services/display_power_service_provider.cc
[modify] https://crrev.com/7f6f2893a7119bc34d59af855bfc3af4124ccb33/chromeos/dbus/services/display_power_service_provider.h
[add] https://crrev.com/7f6f2893a7119bc34d59af855bfc3af4124ccb33/chromeos/dbus/services/org.chromium.DisplayService.conf

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/54c47bfd4ada34545cccae5f96e972511a2c559f

commit 54c47bfd4ada34545cccae5f96e972511a2c559f
Author: Lann Martin <lannm@chromium.org>
Date: Wed Aug 02 22:02:34 2017

autotest: Switch dbus call from LibCrosService to DisplayService

Removed `call_chrome_dbus_method` since LibCrosService is no longer used
in autotest.

BUG= chromium:644319 
TEST=test_that <target> power_Consumption

Change-Id: Icebb1084ff50866fda9f3f84546dd439e1b54910
Reviewed-on: https://chromium-review.googlesource.com/584939
Commit-Ready: Lann Martin <lannm@chromium.org>
Tested-by: Lann Martin <lannm@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/54c47bfd4ada34545cccae5f96e972511a2c559f/client/cros/power_utils.py

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/3988e299c8719a7b4f6691869cbc440eb34edbea

commit 3988e299c8719a7b4f6691869cbc440eb34edbea
Author: Lann Martin <lannm@chromium.org>
Date: Thu Aug 03 16:04:13 2017

power: Switch from DBus LibCrosService to DisplayService

Changed various names and comments from Chrome to DisplayService to
reflect DisplayService being moved out of the chrome process.

BUG= chromium:644319 
TEST=unittests; manually verified power button turns off backlight
CQ-DEPEND=CL:581938

Change-Id: I82e406ffbf341f381f0e7c20d4eb8a9a83cc9edc
Reviewed-on: https://chromium-review.googlesource.com/585348
Commit-Ready: Lann Martin <lannm@chromium.org>
Tested-by: Lann Martin <lannm@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/3988e299c8719a7b4f6691869cbc440eb34edbea/power_manager/powerd/system/display/display_power_setter.h
[modify] https://crrev.com/3988e299c8719a7b4f6691869cbc440eb34edbea/power_manager/powerd/policy/backlight_controller_stub.cc
[modify] https://crrev.com/3988e299c8719a7b4f6691869cbc440eb34edbea/power_manager/powerd/policy/external_backlight_controller_unittest.cc
[modify] https://crrev.com/3988e299c8719a7b4f6691869cbc440eb34edbea/power_manager/powerd/policy/internal_backlight_controller.h
[modify] https://crrev.com/3988e299c8719a7b4f6691869cbc440eb34edbea/power_manager/powerd/policy/backlight_controller_stub.h
[modify] https://crrev.com/3988e299c8719a7b4f6691869cbc440eb34edbea/power_manager/powerd/policy/internal_backlight_controller.cc
[modify] https://crrev.com/3988e299c8719a7b4f6691869cbc440eb34edbea/power_manager/powerd/policy/internal_backlight_controller_unittest.cc
[modify] https://crrev.com/3988e299c8719a7b4f6691869cbc440eb34edbea/power_manager/powerd/daemon.cc
[modify] https://crrev.com/3988e299c8719a7b4f6691869cbc440eb34edbea/power_manager/powerd/policy/external_backlight_controller.h
[modify] https://crrev.com/3988e299c8719a7b4f6691869cbc440eb34edbea/power_manager/powerd/policy/keyboard_backlight_controller.cc
[modify] https://crrev.com/3988e299c8719a7b4f6691869cbc440eb34edbea/power_manager/powerd/policy/keyboard_backlight_controller.h
[modify] https://crrev.com/3988e299c8719a7b4f6691869cbc440eb34edbea/power_manager/powerd/policy/backlight_controller.h
[modify] https://crrev.com/3988e299c8719a7b4f6691869cbc440eb34edbea/power_manager/powerd/policy/external_backlight_controller.cc
[modify] https://crrev.com/3988e299c8719a7b4f6691869cbc440eb34edbea/power_manager/powerd/daemon.h
[modify] https://crrev.com/3988e299c8719a7b4f6691869cbc440eb34edbea/power_manager/powerd/daemon_unittest.cc
[modify] https://crrev.com/3988e299c8719a7b4f6691869cbc440eb34edbea/power_manager/powerd/system/display/display_power_setter.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/frecon/+/059fe66fb49d32e236d358d754d988d4602d9f79

commit 059fe66fb49d32e236d358d754d988d4602d9f79
Author: Lann Martin <lannm@chromium.org>
Date: Thu Aug 03 16:04:14 2017

frecon: Switch from org.chromium.LibCrosService to .DisplayService

These methods are being migrated out of the chrome process.

BUG= chromium:644319 
TEST=boot pyro, ctrl+alt+F1/F2 to check Take/ReleaseDisplay

Change-Id: I0da35ff177fa61852f288f464b088cdcff309bf5
Reviewed-on: https://chromium-review.googlesource.com/583441
Commit-Ready: Lann Martin <lannm@chromium.org>
Tested-by: Lann Martin <lannm@chromium.org>
Reviewed-by: Dominik Behr <dbehr@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/059fe66fb49d32e236d358d754d988d4602d9f79/dbus_interface.h
[modify] https://crrev.com/059fe66fb49d32e236d358d754d988d4602d9f79/dbus.c

The only task left is to remove the LibCrosService methods; how long should I wait for that? Until the next Chrome dev release?

I think that the closest thing we have to a rule here is to wait "long enough" for most Chrome OS UI developers to stop using older versions of the OS in conjunction with ToT versions of Chrome that they've built themselves. stevenjb@ and I arrived at 1-2 weeks after hours* of tense negotiations, IIRC.

* not really

Alrighty, I'll give it 2 weeks.

The NextAction date has arrived: 2017-08-16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d0a4305ecb24d0dccfff18eea6748d6b56db6ac6

commit d0a4305ecb24d0dccfff18eea6748d6b56db6ac6
Author: Lann Martin <lannm@chromium.org>
Date: Wed Aug 23 20:03:14 2017

dbus: Remove display methods from LibCrosService

These methods have been migrated to org.chromium.DisplayService.

BUG= chromium:644319 
TEST=deploy to pyro; test power button and console switching
CQ-DEPEND=CL:583441
CQ-DEPEND=CL:584939
CQ-DEPEND=CL:585348

Change-Id: Ia504a0259f72ee8c9872c49356f86fd1e830a9c2
Reviewed-on: https://chromium-review.googlesource.com/587031
Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Lann Martin <lannm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496778}
[modify] https://crrev.com/d0a4305ecb24d0dccfff18eea6748d6b56db6ac6/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
[modify] https://crrev.com/d0a4305ecb24d0dccfff18eea6748d6b56db6ac6/chromeos/dbus/services/console_service_provider.cc
[modify] https://crrev.com/d0a4305ecb24d0dccfff18eea6748d6b56db6ac6/chromeos/dbus/services/console_service_provider.h
[modify] https://crrev.com/d0a4305ecb24d0dccfff18eea6748d6b56db6ac6/chromeos/dbus/services/display_power_service_provider.cc
[modify] https://crrev.com/d0a4305ecb24d0dccfff18eea6748d6b56db6ac6/chromeos/dbus/services/display_power_service_provider.h
[modify] https://crrev.com/d0a4305ecb24d0dccfff18eea6748d6b56db6ac6/chromeos/dbus/services/org.chromium.LibCrosService.conf

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/frecon/+/3e6a0b5d87bea47ce50bf007a7659cdd61108220

commit 3e6a0b5d87bea47ce50bf007a7659cdd61108220
Author: Lann Martin <lannm@chromium.org>
Date: Thu Sep 21 17:16:27 2017

frecon: Switch from org.chromium.LibCrosService to .DisplayService

These methods are being migrated out of the chrome process.

BUG= chromium:644319 
TEST=boot pyro, ctrl+alt+F1/F2 to check Take/ReleaseDisplay

Change-Id: I0da35ff177fa61852f288f464b088cdcff309bf5
Reviewed-on: https://chromium-review.googlesource.com/583441
Commit-Ready: Lann Martin <lannm@chromium.org>
Tested-by: Lann Martin <lannm@chromium.org>
Reviewed-by: Dominik Behr <dbehr@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
(cherry picked from commit 059fe66fb49d32e236d358d754d988d4602d9f79)
Reviewed-on: https://chromium-review.googlesource.com/677060
Commit-Queue: Dominik Behr <dbehr@chromium.org>
Tested-by: Dominik Behr <dbehr@chromium.org>

[modify] https://crrev.com/3e6a0b5d87bea47ce50bf007a7659cdd61108220/dbus_interface.h
[modify] https://crrev.com/3e6a0b5d87bea47ce50bf007a7659cdd61108220/dbus.c

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/frecon/+/a2ef308f3b9a3efb714ea58f8485f9f94bd8ca74

commit a2ef308f3b9a3efb714ea58f8485f9f94bd8ca74
Author: Lann Martin <lannm@chromium.org>
Date: Wed Sep 27 21:26:29 2017

Revert "frecon: Switch from org.chromium.LibCrosService to .DisplayService"

This reverts commit 3e6a0b5d87bea47ce50bf007a7659cdd61108220.

Reason for revert:  crbug.com/768466 

Original change's description:
> frecon: Switch from org.chromium.LibCrosService to .DisplayService
> 
> These methods are being migrated out of the chrome process.
> 
> BUG= chromium:644319 
> TEST=boot pyro, ctrl+alt+F1/F2 to check Take/ReleaseDisplay
> 
> Change-Id: I0da35ff177fa61852f288f464b088cdcff309bf5
> Reviewed-on: https://chromium-review.googlesource.com/583441
> Commit-Ready: Lann Martin <lannm@chromium.org>
> Tested-by: Lann Martin <lannm@chromium.org>
> Reviewed-by: Dominik Behr <dbehr@chromium.org>
> Reviewed-by: Dan Erat <derat@chromium.org>
> (cherry picked from commit 059fe66fb49d32e236d358d754d988d4602d9f79)
> Reviewed-on: https://chromium-review.googlesource.com/677060
> Commit-Queue: Dominik Behr <dbehr@chromium.org>
> Tested-by: Dominik Behr <dbehr@chromium.org>

Bug:  chromium:644319 
Change-Id: Ide19638e66f890ab235929cd0920758602036b84
Reviewed-on: https://chromium-review.googlesource.com/687954
Reviewed-by: Lann Martin <lannm@chromium.org>
Reviewed-by: Dominik Behr <dbehr@google.com>
Tested-by: Lann Martin <lannm@chromium.org>
Tested-by: Dominik Behr <dbehr@google.com>
Commit-Queue: Dominik Behr <dbehr@google.com>
Trybot-Ready: Dominik Behr <dbehr@google.com>

[modify] https://crrev.com/a2ef308f3b9a3efb714ea58f8485f9f94bd8ca74/dbus_interface.h
[modify] https://crrev.com/a2ef308f3b9a3efb714ea58f8485f9f94bd8ca74/dbus.c

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2a1714b8b38810c04e5165c6e482c3e08a4231b6

commit 2a1714b8b38810c04e5165c6e482c3e08a4231b6
Author: Daniel Erat <derat@chromium.org>
Date: Fri Mar 30 19:10:32 2018

chromeos: Remove refs to old display power D-Bus constants.

Update DisplayPowerServiceProvider to use method names from
org.chromium.DisplayService rather than ones that used to be
exported by org.chromium.LibCrosService.

Bug:  644319 
Change-Id: Ifdf065b860173a53b749505d1af3911dc31419a4
Reviewed-on: https://chromium-review.googlesource.com/987239
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547229}
[modify] https://crrev.com/2a1714b8b38810c04e5165c6e482c3e08a4231b6/chromeos/dbus/services/display_power_service_provider.cc

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chromeos/overlays/overlay-eve-campfire-private/+/075cfb345d3c4f7f4ed2a3590f2ebcec2dfd1ac6

commit 075cfb345d3c4f7f4ed2a3590f2ebcec2dfd1ac6
Author: Daniel Erat <derat@chromium.org>
Date: Mon Apr 09 19:42:40 2018