Issue 602686  has been merged into this issue.

A nice start might be to make the shutdown button in the ash system tray work properly.

(There will probably be shutdown crashes in chrome and ash, but it would be good to know about those.)

Re #3, is ash currently not instantiating a DBusThreadManager at all?

Ash creates one here:
https://cs.chromium.org/chromium/src/ash/mus/window_manager_application.cc?type=cs&q=f:mus+dbus&sq=package:chromium&l=47

I'm looking at making ash only initialize a subset of dbus clients, see  issue 644414 . For now, though, it's initializing everything, including powerd stuff.

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

commit bc32dfbb9d481c8d93efe038569f3a6d81267db5
Author: msw <msw@chromium.org>
Date: Thu Sep 15 00:52:17 2016

mash: Fix Chrome crash accessing chrome://settings.

Avoid Shell and PowerStatus access for display/power options.

BUG= 646558 , 548429 ,644348
TEST=No crash accessing chrome://settings in chrome --mash.
R=stevenjb@chromium.org

Review-Url: https://codereview.chromium.org/2341783005
Cr-Commit-Position: refs/heads/master@{#418731}

[modify] https://crrev.com/bc32dfbb9d481c8d93efe038569f3a6d81267db5/chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
[modify] https://crrev.com/bc32dfbb9d481c8d93efe038569f3a6d81267db5/chrome/browser/ui/webui/options/chromeos/power_handler.cc

Is this primarily a tracking bug? I'm trying to figure out what needs to be done at this point.

James, have you decided what route you're advocating for clients that probably need to be present in both arc and Chrome? If most of the code will used by both processes, should I add an enum parameter to the client class's c'tor with values like ASH, BROWSER, and BOTH? That seems not-great, in that it's tough for a caller to know whether a given method will be callable or not. Do you prefer for this code to be split into multiple clients instead?

+stevenjb, hashimoto. I'm working right now on how to split up dbus clients between processes. WIP CL: https://codereview.chromium.org/2343993003

This is mostly a tracking bug. I don't personally know powerd well enough to know what won't work.

Any thoughts on how to handle ash::PowerStatus?

I'm working on some code in SystemTrayDelegateChromeos that uses ash::PowerStatus::Get(). This particular piece of code I can migrate into ash, but it seems like both the ash and the browser process will need to know about battery state. Could PowerStatus be factored into someplace shared (down in //chromeos/) and get updated by powerd in both processes?

#9: ash::PowerStatus currently mostly just caches the latest PowerSupplyProperties proto received from powerd and provides convenience methods on top of it. Many of those methods are very UI-specific, though (strings, icons, ugh even a display dependency to get the scale factor, etc.), so I think it might be hard to move it into chromeos/ without splitting the UI-specific bits into another class that lives somewhere else.

After glancing through the code, I'm not convinced that the browser process needs this. I see chrome/browser/ui/ash/system_tray_delegate_chromeos.cc using it, but that looks like it's maybe just to display the power settings on behalf of ash.

I don't know what the plan is for where settings should live, but there _is_ a PowerStatus::SetPowerSource() method that calls powerd via PowerManagerClient. It looks like it's only used by chrome/browser/ui/webui/options/chromeos/power_handler.cc, and there's maybe not any good reason for it to exist.

Hmm. We've got //ash/shared for simple code that can be shared between //chrome and //ash, but I see PowerState also has an ash/common/material_design_controller.h dependency.

WebUI settings will continue to live in Chrome, so we're going to need some way to poke powerd from WebUI, then get an update in ash.

More-concrete proposal:

a) The UI-independent parts of PowerStatus should live under //chromeos.
b) GetAccessibleNameString and everything with *BatteryImage* in its name should live in //ash since I think that's the only place they're used.
c) PowerStatus::SetPowerSource() should go away because it's just a wrapper around PowerManagerClient's method. The same argument could be made for PowerStatus::RequestStatusUpdate().

For a), it seems reasonable (and maybe desirable) to rename the class to something like PowerStatusWatcher and make it not be a singleton. If we keep it as a singleton, it's basically 1:1 with PowerManagerClient (registers as an observer of it and caches/interprets a proto from it).

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

commit d2de27ad565eae8ca5e3c61cc055ecb34e4e1b3f
Author: jamescook <jamescook@chromium.org>
Date: Thu Sep 29 20:31:08 2016

Support SystemTrayDelegate::ShowPowerSettings() in mustash

* Wire through SystemTrayDelegateMus -> mojom::SystemTrayClient -> SystemTrayCommon
* To avoid ash dependencies in SystemTrayCommon, check for a battery in the //ash
code, which has access to ash::PowerStatus
* Remove the check for the --enable-power-overlay switch, since this code is only
triggered from DualRoleNotification in ash, which only occurs on a real device

BUG=644348, 647412 
TEST=manual, change the code to call ShowPowerSettings when clicking on the
tray date and observe that power settings opens

Review-Url: https://codereview.chromium.org/2377703003
Cr-Commit-Position: refs/heads/master@{#421923}

[modify] https://crrev.com/d2de27ad565eae8ca5e3c61cc055ecb34e4e1b3f/ash/mus/system_tray_delegate_mus.cc
[modify] https://crrev.com/d2de27ad565eae8ca5e3c61cc055ecb34e4e1b3f/ash/public/interfaces/system_tray.mojom
[modify] https://crrev.com/d2de27ad565eae8ca5e3c61cc055ecb34e4e1b3f/chrome/browser/ui/ash/system_tray_client.cc
[modify] https://crrev.com/d2de27ad565eae8ca5e3c61cc055ecb34e4e1b3f/chrome/browser/ui/ash/system_tray_client.h
[modify] https://crrev.com/d2de27ad565eae8ca5e3c61cc055ecb34e4e1b3f/chrome/browser/ui/ash/system_tray_common.cc
[modify] https://crrev.com/d2de27ad565eae8ca5e3c61cc055ecb34e4e1b3f/chrome/browser/ui/ash/system_tray_common.h
[modify] https://crrev.com/d2de27ad565eae8ca5e3c61cc055ecb34e4e1b3f/chrome/browser/ui/ash/system_tray_delegate_chromeos.cc

FYI I'm doing some work to move more shutdown handling code out of chrome and into ash as part of my work on  issue 647412  (SystemTrayDelegate for mustash).

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

commit 04570b6023aeada35ead88f6488618be307793d1
Author: jamescook <jamescook@chromium.org>
Date: Thu Nov 10 19:51:59 2016

chromeos: Move SystemTrayDelegate::RequestShutdown to WmShell

For mustash the chrome browser process cannot call directly into ash. The
implementation of this method in chrome's SystemTrayDelegateChromeos just
turns around and calls into ash::LockStateController.

Moving the method allows ash to process these requests internally. The mus
implementation is still NOTIMPLEMENTED because we don't have mus support
for LockStateController and its animations.

After removing another unused method this results in SystemTrayDelegateChromeos
having no ash::Shell references. (It still uses ash::WmShell, though.)

This is part 1 of moving shutdown handling out of chrome into ash.

BUG=644348,  647412 
TEST=Shutdown from system tray still works in classic ash

Review-Url: https://codereview.chromium.org/2491403003
Cr-Commit-Position: refs/heads/master@{#431323}

[modify] https://crrev.com/04570b6023aeada35ead88f6488618be307793d1/ash/aura/wm_shell_aura.cc
[modify] https://crrev.com/04570b6023aeada35ead88f6488618be307793d1/ash/aura/wm_shell_aura.h
[modify] https://crrev.com/04570b6023aeada35ead88f6488618be307793d1/ash/common/system/date/date_default_view.cc
[modify] https://crrev.com/04570b6023aeada35ead88f6488618be307793d1/ash/common/system/tiles/tiles_default_view.cc
[modify] https://crrev.com/04570b6023aeada35ead88f6488618be307793d1/ash/common/system/tray/system_tray_delegate.cc
[modify] https://crrev.com/04570b6023aeada35ead88f6488618be307793d1/ash/common/system/tray/system_tray_delegate.h
[modify] https://crrev.com/04570b6023aeada35ead88f6488618be307793d1/ash/common/wm_shell.h
[modify] https://crrev.com/04570b6023aeada35ead88f6488618be307793d1/ash/mus/bridge/wm_shell_mus.cc
[modify] https://crrev.com/04570b6023aeada35ead88f6488618be307793d1/ash/mus/bridge/wm_shell_mus.h
[modify] https://crrev.com/04570b6023aeada35ead88f6488618be307793d1/chrome/browser/ui/ash/system_tray_delegate_chromeos.cc
[modify] https://crrev.com/04570b6023aeada35ead88f6488618be307793d1/chrome/browser/ui/ash/system_tray_delegate_chromeos.h
[modify] https://crrev.com/04570b6023aeada35ead88f6488618be307793d1/chrome/browser/ui/ash/system_tray_delegate_chromeos_browsertest_chromeos.cc

Deprecating Proj-Mustash-Mus-WS label in favor of Components.

Bug label cleanup.

PowerPrefs got moved to ash by https://crrev.com/c/986889 for issue 812504. I think that the main thing remaining here is to make sure that all wake locks get consolidated in the PowerPrefs instance owned by ash. Wake locks go through the device service, IIRC, but I'm not sure of the current state (fate?) of that service. If it lives in ash, this might just work already.