Support SystemTrayDelegate::ShowPowerSettings() in mustash

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

Committed: https://crrev.com/d2de27ad565eae8ca5e3c61cc055ecb34e4e1b3f
Cr-Commit-Position: refs/heads/master@{#421923}

[James Cook, 2016-09-27 23:18:37]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 23:18:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2016-09-27 23:23:12]

jamescook@chromium.org changed reviewers:
+ michaelpg@chromium.org

[James Cook, 2016-09-27 23:23:13]

michaelpg, please take a look.

If you need mustash background, see go/mustash-intro and go/mustash-ui. (I'll be
broadcasting those more widely late this week, they aren't quite done.)

https://codereview.chromium.org/2377703003/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/power/dual_role_notification.cc (right):

https://codereview.chromium.org/2377703003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/power/dual_role_notification.cc:40: if
(PowerStatus::Get()->IsBatteryPresent())
I'm unsure if we still need this check (I moved it here from
SystemTrayDelegateChromeos). It wasn't clear to me if it made sense to show the
settings if you didn't have a battery.

https://codereview.chromium.org/2377703003/diff/1/chrome/browser/ui/ash/syste...
File chrome/browser/ui/ash/system_tray_common.cc (right):

https://codereview.chromium.org/2377703003/diff/1/chrome/browser/ui/ash/syste...
chrome/browser/ui/ash/system_tray_common.cc:61: void
SystemTrayCommon::ShowPowerSettings() {
The reason for moving the if() conditions into //ash is that this file can't
depend on ash. (Under mustash, the ash code runs in a different process than the
browser, so browser can't poke directly into ash code.)

https://codereview.chromium.org/2377703003/diff/1/chrome/browser/ui/ash/syste...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (left):

https://codereview.chromium.org/2377703003/diff/1/chrome/browser/ui/ash/syste...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:451:
ash::PowerStatus::Get()->SupportsDualRoleDevices()))) {
I didn't check SupportsDualRoleDevices() since this function is only called from
a dual-role notification.

[commit-bot: I haz the power, 2016-09-28 00:11:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 00:11:45]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-09-28 21:56:10]

Ping michaelpg? Sorry to bug you, but I've got a couple patches stacked up
behind this one.

[michaelpg, 2016-09-28 23:09:15]

https://codereview.chromium.org/2377703003/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/power/dual_role_notification.cc (right):

https://codereview.chromium.org/2377703003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/power/dual_role_notification.cc:40: if
(PowerStatus::Get()->IsBatteryPresent())
On 2016/09/27 23:23:13, James Cook wrote:
> I'm unsure if we still need this check (I moved it here from
> SystemTrayDelegateChromeos). It wasn't clear to me if it made sense to show
the
> settings if you didn't have a battery.

We shouldn't show the *notification* if there isn't a battery. I believe that's
already the case from the power settings proto. So this check isn't needed in
the notification itself.

https://codereview.chromium.org/2377703003/diff/1/ash/mus/system_tray_delegat...
File ash/mus/system_tray_delegate_mus.cc (right):

https://codereview.chromium.org/2377703003/diff/1/ash/mus/system_tray_delegat...
ash/mus/system_tray_delegate_mus.cc:82:
ConnectToSystemTrayClient()->ShowPowerSettings();
does this mean power_handler.cc be updated as well to support mash?
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/chromeos...

If not, then this would just open Settings for no reason.

[James Cook, 2016-09-29 15:28:29]

https://codereview.chromium.org/2377703003/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/power/dual_role_notification.cc (right):

https://codereview.chromium.org/2377703003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/power/dual_role_notification.cc:40: if
(PowerStatus::Get()->IsBatteryPresent())
On 2016/09/28 23:09:15, michaelpg wrote:
> On 2016/09/27 23:23:13, James Cook wrote:
> > I'm unsure if we still need this check (I moved it here from
> > SystemTrayDelegateChromeos). It wasn't clear to me if it made sense to show
> the
> > settings if you didn't have a battery.
> 
> We shouldn't show the *notification* if there isn't a battery. I believe
that's
> already the case from the power settings proto. So this check isn't needed in
> the notification itself.

Removed the if().

https://codereview.chromium.org/2377703003/diff/1/ash/mus/system_tray_delegat...
File ash/mus/system_tray_delegate_mus.cc (right):

https://codereview.chromium.org/2377703003/diff/1/ash/mus/system_tray_delegat...
ash/mus/system_tray_delegate_mus.cc:82:
ConnectToSystemTrayClient()->ShowPowerSettings();
On 2016/09/28 23:09:15, michaelpg wrote:
> does this mean power_handler.cc be updated as well to support mash?
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/chromeos...
> 
> If not, then this would just open Settings for no reason.

Yes, power_handler.cc will need to be updated to support mash, but I would
prefer to do that in a separate CL. Updating PowerHandler will require
initializing PowerStatus in the ash process, which will trigger additional D-Bus
traffic to powerd via PowerManagerClient. I am not completely certain that I can
do that safely with PowerStatus as it exists today. See discussion in
crbug.com/644348

Right now under mash this CL opens battery status with battery info at 50%,
which I suspect is just hard-coded into the HTML as the default. It doesn't
update, but it does open something.

If you would prefer me to wait on this CL until PowerStatus is sorted out I'm
happy to do that.

[michaelpg, 2016-09-29 17:00:51]

lgtm

https://codereview.chromium.org/2377703003/diff/1/ash/mus/system_tray_delegat...
File ash/mus/system_tray_delegate_mus.cc (right):

https://codereview.chromium.org/2377703003/diff/1/ash/mus/system_tray_delegat...
ash/mus/system_tray_delegate_mus.cc:82:
ConnectToSystemTrayClient()->ShowPowerSettings();
On 2016/09/29 15:28:29, James Cook wrote:
> On 2016/09/28 23:09:15, michaelpg wrote:
> > does this mean power_handler.cc be updated as well to support mash?
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/chromeos...
> > 
> > If not, then this would just open Settings for no reason.
> 
> Yes, power_handler.cc will need to be updated to support mash, but I would
> prefer to do that in a separate CL. Updating PowerHandler will require
> initializing PowerStatus in the ash process, which will trigger additional
D-Bus
> traffic to powerd via PowerManagerClient. I am not completely certain that I
can
> do that safely with PowerStatus as it exists today. See discussion in
> crbug.com/644348
> 
> Right now under mash this CL opens battery status with battery info at 50%,
> which I suspect is just hard-coded into the HTML as the default. It doesn't
> update, but it does open something.
> 
> If you would prefer me to wait on this CL until PowerStatus is sorted out I'm
> happy to do that.

The only reason to open Settings from the notification (and the only reason to
show the notification) is to give the user the ability to change power source.

On the other hand, I guess this is better than a NOTIMPLEMENTED(). So I'll leave
it up to you what order you want to land things in.

[James Cook, 2016-09-29 18:30:40]

I think I'll land this first, as it will give me another way to test
PowerHandler / PowerManager when I get that running.

Thanks for the review.

[James Cook, 2016-09-29 18:31:08]

jamescook@chromium.org changed reviewers:
+ tsepez@chromium.org

[James Cook, 2016-09-29 18:31:08]

tsepez, can you look at //ash/public/interfaces/system_tray.mojom ?

[Tom Sepez, 2016-09-29 18:33:26]

lgtm

[James Cook, 2016-09-29 19:15:29]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2016-09-29 19:15:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 19:25:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 19:25:31]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[James Cook, 2016-09-29 19:27:58]

jamescook@chromium.org changed reviewers:
+ msw@chromium.org

[James Cook, 2016-09-29 19:27:58]

msw, can I get OWNERS for c/b/ui/ash ?

[msw, 2016-09-29 20:22:36]

lgtm

[James Cook, 2016-09-29 20:24:34]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2016-09-29 20:24:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 20:31:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-29 20:33:44]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/d2de27ad565eae8ca5e3c61cc055ecb34e4e1b3f
Cr-Commit-Position: refs/heads/master@{#421923}
==========

[commit-bot: I haz the power, 2016-09-29 20:33:45]

Patchset 3 (id:??) landed as
https://crrev.com/d2de27ad565eae8ca5e3c61cc055ecb34e4e1b3f
Cr-Commit-Position: refs/heads/master@{#421923}