New issue
Advanced search Search tips

Issue 828151 link

Starred by 1 user
Status: Fixed
Owner:
derat@chromium.org
Closed: Aug 15
Cc:
ejcaruso@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Task

Blocking:
issue 850807



Sign in to add a comment

Move D-Bus code out of powerd's Daemon class where possible

Project Member Reported by derat@chromium.org, Apr 2 2018 
https://crrev.com/c/989214 adds some code to powerd's DBusWrapper class to make it possible for arbitrary classes to directly watch for D-Bus service name ownership changes.

At present, most D-Bus work gets done by the Daemon class. That class is huge and hard to test, and I should move its D-Bus code into other classes where possible.

Some D-Bus code will probably remain in Daemon, particularly for cases where multiple classes want to watch for the same signal.
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 5 2018 

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

commit 1451637548baabc8079a99d86485ace637142948
Author: Daniel Erat <derat@chromium.org>
Date: Thu Apr 05 01:52:02 2018

power: Move CRAS D-Bus code into AudioClient.

Move code for watching for CRAS restarts and registering for
its D-Bus signals out of the Daemon class and into
AudioClient, the class that actually consumes this
information.

Also update DBusWatcherStub to support notifying observers
about name ownership changes and handling outgoing D-Bus
method calls from powerd, and add unit tests to exercise
AudioClient's D-Bus code.

BUG= chromium:753596 , chromium:828151 
TEST=unit tests pass; also watched
     /var/log/power_manager/powerd.LATEST and verified that
     audio activity start/stop and headphone plug/unplug
     events are reported

Change-Id: If315bf85cf1646893d6be85ca9adfc7c031ef973
Reviewed-on: https://chromium-review.googlesource.com/991197
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>

[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/system/dbus_wrapper_stub.h
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/system/audio_client.cc
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/power_manager.gyp
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/system/audio_client_interface.h
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/system/dbus_wrapper_stub.cc
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/daemon.cc
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/system/input_watcher_unittest.cc
[add] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/system/audio_client_unittest.cc
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/system/audio_client.h
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/daemon.h
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/daemon_unittest.cc
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/system/audio_client_stub.cc
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/system/audio_client_stub.h
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 18 2018 

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

commit bc3ad7d369ae59f627f9094bb84ca3a8bb18d9c3
Author: Daniel Erat <derat@chromium.org>
Date: Wed Apr 18 03:53:25 2018

power: Move D-Bus code to InputEventHandler.

Move powerd's code for handling
HandlePowerButtonAcknowledgment, IgnoreNextPowerButtonPress,
and GetSwitchStates D-Bus method calls from the Daemon class
to InputEventHandler.

BUG= chromium:828151 
TEST=unit tests still pass; also enabled verbose logging and
     checked that all three methods still work as expected
     when called

Change-Id: Ibadc6427bc8117ff15e486dba4d209a69c581ef4
Reviewed-on: https://chromium-review.googlesource.com/1016005
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>

[modify] https://crrev.com/bc3ad7d369ae59f627f9094bb84ca3a8bb18d9c3/power_manager/powerd/policy/input_event_handler.cc
[modify] https://crrev.com/bc3ad7d369ae59f627f9094bb84ca3a8bb18d9c3/power_manager/powerd/system/dbus_wrapper_stub.h
[modify] https://crrev.com/bc3ad7d369ae59f627f9094bb84ca3a8bb18d9c3/power_manager/powerd/policy/input_event_handler_unittest.cc
[modify] https://crrev.com/bc3ad7d369ae59f627f9094bb84ca3a8bb18d9c3/power_manager/powerd/policy/input_event_handler.h
[modify] https://crrev.com/bc3ad7d369ae59f627f9094bb84ca3a8bb18d9c3/power_manager/powerd/system/dbus_wrapper_stub.cc
[modify] https://crrev.com/bc3ad7d369ae59f627f9094bb84ca3a8bb18d9c3/power_manager/powerd/daemon.cc
[modify] https://crrev.com/bc3ad7d369ae59f627f9094bb84ca3a8bb18d9c3/power_manager/powerd/daemon.h
[modify] https://crrev.com/bc3ad7d369ae59f627f9094bb84ca3a8bb18d9c3/power_manager/powerd/daemon_unittest.cc
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 27 2018 

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

commit a2d268e35ff80c00d4e834f666f6bd84aee1e536
Author: Daniel Erat <derat@chromium.org>
Date: Fri Apr 27 19:22:40 2018

power: Move D-Bus code to PowerSupply.

Move powerd's code for handling GetPowerSupplyProperties and
SetPowerSource D-Bus method calls from the Daemon class to
the PowerSupply class. Emit PowerSupplyPoll signals from
PowerSupply as well.

BUG= chromium:828151 
TEST=updated unit tests pass; also verified that chrome UI
     still reflects correct power status, power source
     switching via chrome://settings works, and that updated
     tools still work

Change-Id: Icff809a40144f8bb2f17dcd502734abcd70c883b
Reviewed-on: https://chromium-review.googlesource.com/1015199
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>

[modify] https://crrev.com/a2d268e35ff80c00d4e834f666f6bd84aee1e536/power_manager/tools/power_supply_info.cc
[modify] https://crrev.com/a2d268e35ff80c00d4e834f666f6bd84aee1e536/power_manager/tools/dump_power_status.cc
[modify] https://crrev.com/a2d268e35ff80c00d4e834f666f6bd84aee1e536/power_manager/tools/backlight_tool.cc
[modify] https://crrev.com/a2d268e35ff80c00d4e834f666f6bd84aee1e536/power_manager/powerd/main.cc
[modify] https://crrev.com/a2d268e35ff80c00d4e834f666f6bd84aee1e536/power_manager/powerd/system/power_supply_unittest.cc
[modify] https://crrev.com/a2d268e35ff80c00d4e834f666f6bd84aee1e536/power_manager/powerd/system/power_supply.h
[modify] https://crrev.com/a2d268e35ff80c00d4e834f666f6bd84aee1e536/power_manager/powerd/daemon_delegate.h
[modify] https://crrev.com/a2d268e35ff80c00d4e834f666f6bd84aee1e536/power_manager/powerd/system/power_supply_stub.h
[modify] https://crrev.com/a2d268e35ff80c00d4e834f666f6bd84aee1e536/power_manager/powerd/daemon.cc
[modify] https://crrev.com/a2d268e35ff80c00d4e834f666f6bd84aee1e536/power_manager/powerd/system/power_supply_stub.cc
[modify] https://crrev.com/a2d268e35ff80c00d4e834f666f6bd84aee1e536/power_manager/powerd/daemon.h
[modify] https://crrev.com/a2d268e35ff80c00d4e834f666f6bd84aee1e536/power_manager/powerd/daemon_unittest.cc
[modify] https://crrev.com/a2d268e35ff80c00d4e834f666f6bd84aee1e536/power_manager/powerd/system/power_supply.cc

Comment 4 by derat@chromium.org, Jun 8 2018

Blocking: 850807
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 20 2018 

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

commit 8ae6dcca2b7464a828d7601b5e844eb953b1d52e
Author: Daniel Erat <derat@chromium.org>
Date: Wed Jun 20 08:43:26 2018

power: Move D-Bus code to BacklightController classes.

Move backlight-related D-Bus code out of powerd's Daemon
class and into BacklightController implementations. This
makes the Daemon class smaller and improves unit test
coverage (since unit tests now exercise D-Bus message
deserialization).

BUG= chromium:828151 
TEST=unit tests pass and manually verified that keyboard and
     panel backlight accelerators still work

Change-Id: I074bcb016c6d7be3b276b35068aee3a07db3a57a
Reviewed-on: https://chromium-review.googlesource.com/1094261
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Abhishek Bhardwaj <abhishekbh@chromium.org>

[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/policy/internal_backlight_controller.h
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/daemon_delegate.h
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/daemon.cc
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/policy/keyboard_backlight_controller.cc
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/daemon.h
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/policy/external_backlight_controller.cc
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/policy/backlight_controller.h
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/main.cc
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/policy/external_backlight_controller_unittest.cc
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/policy/internal_backlight_controller_unittest.cc
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/system/dbus_wrapper_stub.h
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/policy/keyboard_backlight_controller_unittest.cc
[add] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/policy/backlight_controller_test_util.cc
[add] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/policy/backlight_controller.cc
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/policy/backlight_controller_stub.cc
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/policy/backlight_controller_stub.h
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/tools/backlight_tool.cc
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/power_manager.gyp
[add] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/policy/backlight_controller_test_util.h
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/system/dbus_wrapper_stub.cc
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/policy/external_backlight_controller.h
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/policy/keyboard_backlight_controller.h
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/policy/internal_backlight_controller.cc
[modify] https://crrev.com/8ae6dcca2b7464a828d7601b5e844eb953b1d52e/power_manager/powerd/daemon_unittest.cc
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 24 2018 

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

commit c2f802742a73d8eefddaa0ef9f990b29f902c052
Author: Daniel Erat <derat@chromium.org>
Date: Sun Jun 24 18:26:02 2018

power: Move more D-Bus code into StateController.

Move powerd's code for observing and calling update_engine
from Daemon to StateController, the only class that uses it.

Also make StateController handle GetInactivityDelays D-Bus
method calls and emit the following signals itself instead
of being lazy and delegating the work to Daemon:

- IdleActionDeferred
- IdleActionImminent
- InactivityDelaysChanged
- ScreenIdleStateChanged

This improves unit test coverage, as tests now check the
actual messages that are produced rather than just checking
calls to the delegate.

BUG= chromium:828151 
TEST=unit tests pass

Change-Id: I20f80a9f836b74cff088f3a015c50c11f21294a5
Reviewed-on: https://chromium-review.googlesource.com/1111385
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Jia Meng <jiameng@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/c2f802742a73d8eefddaa0ef9f990b29f902c052/power_manager/powerd/system/dbus_wrapper_stub.h
[modify] https://crrev.com/c2f802742a73d8eefddaa0ef9f990b29f902c052/power_manager/common/power_constants.cc
[modify] https://crrev.com/c2f802742a73d8eefddaa0ef9f990b29f902c052/power_manager/powerd/policy/state_controller_unittest.cc
[modify] https://crrev.com/c2f802742a73d8eefddaa0ef9f990b29f902c052/power_manager/common/power_constants.h
[modify] https://crrev.com/c2f802742a73d8eefddaa0ef9f990b29f902c052/power_manager/powerd/system/dbus_wrapper_stub.cc
[modify] https://crrev.com/c2f802742a73d8eefddaa0ef9f990b29f902c052/power_manager/powerd/daemon.cc
[modify] https://crrev.com/c2f802742a73d8eefddaa0ef9f990b29f902c052/power_manager/powerd/policy/state_controller.h
[modify] https://crrev.com/c2f802742a73d8eefddaa0ef9f990b29f902c052/power_manager/powerd/daemon.h
[modify] https://crrev.com/c2f802742a73d8eefddaa0ef9f990b29f902c052/power_manager/powerd/daemon_unittest.cc
[modify] https://crrev.com/c2f802742a73d8eefddaa0ef9f990b29f902c052/power_manager/powerd/policy/state_controller.cc

Comment 7 by derat@chromium.org, Aug 15

Status: Fixed (was: Started)
I'm going to declare victory here. The remaining methods are either best handled by Daemon or have multiple consumers:

  const std::map<const char*, DaemonMethod> kDaemonMethods = {
      {kRequestShutdownMethod, &Daemon::HandleRequestShutdownMethod},
      {kRequestRestartMethod, &Daemon::HandleRequestRestartMethod},
      {kRequestSuspendMethod, &Daemon::HandleRequestSuspendMethod},
      {kHandleVideoActivityMethod, &Daemon::HandleVideoActivityMethod},
      {kHandleUserActivityMethod, &Daemon::HandleUserActivityMethod},
      {kSetIsProjectingMethod, &Daemon::HandleSetIsProjectingMethod},
      {kSetPolicyMethod, &Daemon::HandleSetPolicyMethod},
      {kSetBacklightsForcedOffMethod,
       &Daemon::HandleSetBacklightsForcedOffMethod},
      {kGetBacklightsForcedOffMethod,
       &Daemon::HandleGetBacklightsForcedOffMethod},
  };
►

Sign in to add a comment