chromeos: Refactor system tray code into PowerStatus.

chromeos: Refactor system tray code into PowerStatus.

This attempts to clean up shared Ash system tray code
related to displaying the battery status by moving it into
the PowerStatus class. Other classes no longer use the
PowerSupplyStatus struct, and in a following change, I plan
to remove that struct entirely.

BUG=254173
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209342

[Daniel Erat, 2013-06-28 21:43:23]

https://codereview.chromium.org/18176013/diff/3001/ash/system/chromeos/power/...
File ash/system/chromeos/power/power_status_view.cc (right):

https://codereview.chromium.org/18176013/diff/3001/ash/system/chromeos/power/...
ash/system/chromeos/power/power_status_view.cc:68: icon_->SetImage(
I don't think there's much value to caching the old icon information so we avoid
resetting the same icon unnecessarily -- it adds complexity, and status updates
usually only occur every thirty seconds.

https://codereview.chromium.org/18176013/diff/3001/chromeos/dbus/power_manage...
File chromeos/dbus/power_manager_client.cc (right):

https://codereview.chromium.org/18176013/diff/3001/chromeos/dbus/power_manage...
chromeos/dbus/power_manager_client.cc:807: if (pause_count_ == 2)
The old code was switching immediately to discharging/charging after hitting
100/0, resulting in the "full" state never actually being shown.

https://codereview.chromium.org/18176013/diff/3001/chromeos/dbus/power_manage...
chromeos/dbus/power_manager_client.cc:813: battery_percentage_ +=
(battery_percentage_ >= 10 ? 10 : 1);
The discontinuity in the old code was bugging me: it'd go 100, 90, 80, ... 10,
9, 8, ... 2, 1, 2, ... 8, 9, 10, 11, 21, 31, etc. This makes us use multiples of
10 on the way up as well.

[Daniel Erat, 2013-06-28 22:15:05]

Looks like base/time.h got moved since I uploaded this. New version on the
way...

[James Cook, 2013-06-28 22:23:51]

LGTM - nice patch

[Daniel Erat, 2013-06-28 22:24:16]

update for base/time/time.h

[stevenjb, 2013-06-28 22:50:48]

https://codereview.chromium.org/18176013/diff/3001/ash/system/chromeos/power/...
File ash/system/chromeos/power/power_status.h (right):

https://codereview.chromium.org/18176013/diff/3001/ash/system/chromeos/power/...
ash/system/chromeos/power/power_status.h:58: }
nit: maybe put at the end of the public methods?

https://codereview.chromium.org/18176013/diff/3001/ash/system/chromeos/power/...
ash/system/chromeos/power/power_status.h:60: // Adds or removes an observer.
nit: '// chromeos::PowerManagerClient::Observer'

https://codereview.chromium.org/18176013/diff/3001/ash/system/chromeos/power/...
ash/system/chromeos/power/power_status.h:96: bool IsMainsChargerConnected()
const;
s/Mains/Main ?

https://codereview.chromium.org/18176013/diff/3001/ash/system/chromeos/power/...
File ash/system/chromeos/power/power_status_unittest.cc (right):

https://codereview.chromium.org/18176013/diff/3001/ash/system/chromeos/power/...
ash/system/chromeos/power/power_status_unittest.cc:31: virtual void
OnPowerStatusChanged() OVERRIDE { ++power_changed_count_; }
nit: We really shouldn't inline virtuals...

[stevenjb, 2013-06-28 22:51:14]

lgtm btw

[Daniel Erat, 2013-06-28 23:22:42]

https://codereview.chromium.org/18176013/diff/3001/ash/system/chromeos/power/...
File ash/system/chromeos/power/power_status.h (right):

https://codereview.chromium.org/18176013/diff/3001/ash/system/chromeos/power/...
ash/system/chromeos/power/power_status.h:58: }
On 2013/06/28 22:50:48, stevenjb (chromium) wrote:
> nit: maybe put at the end of the public methods?

(leaving this here as discussed in person, since I've frequently seen inline
methods come after ctor/dtor/static, testing or not)

https://codereview.chromium.org/18176013/diff/3001/ash/system/chromeos/power/...
ash/system/chromeos/power/power_status.h:60: // Adds or removes an observer.
On 2013/06/28 22:50:48, stevenjb (chromium) wrote:
> nit: '// chromeos::PowerManagerClient::Observer'

removed 'virtual' instead

https://codereview.chromium.org/18176013/diff/3001/ash/system/chromeos/power/...
ash/system/chromeos/power/power_status.h:96: bool IsMainsChargerConnected()
const;
On 2013/06/28 22:50:48, stevenjb (chromium) wrote:
> s/Mains/Main ?

"Mains" matches the terminology in the kernel
(https://en.wikipedia.org/wiki/Mains_electricity). for what it's worth, i'll
probably get rid of this method later

https://codereview.chromium.org/18176013/diff/3001/ash/system/chromeos/power/...
File ash/system/chromeos/power/power_status_unittest.cc (right):

https://codereview.chromium.org/18176013/diff/3001/ash/system/chromeos/power/...
ash/system/chromeos/power/power_status_unittest.cc:31: virtual void
OnPowerStatusChanged() OVERRIDE { ++power_changed_count_; }
On 2013/06/28 22:50:48, stevenjb (chromium) wrote:
> nit: We really shouldn't inline virtuals...

note that this isn't a header (or do you just mean that you want the body to be
on its own line?)

[Daniel Erat, 2013-06-28 23:30:38]

remove virtual

[commit-bot: I haz the power, 2013-06-28 23:32:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/18176013/3002

[commit-bot: I haz the power, 2013-06-29 20:56:53]

Change committed as 209342