Only update system power indicators on change

Only update system power indicators on change

For chromeos, this saves us having to recomposite the desktop every time
we get a power status notification (about 1/30 hz).  Saves power, ironically.

BUG=chrome-os-partner:13141
TEST=build, run on lumpy, lucas

Change-Id: Ief427d42e8e450259cbc29ed939182e65008269e


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156474

[sheu, 2012-09-04 18:56:14]

Todd/Sam, please review -- thanks.

[tbroch1, 2012-09-04 21:18:56]

lgtm

[commit-bot: I haz the power, 2012-09-04 21:20:22]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Sam Leffler, 2012-09-04 21:45:01]

lgtm too but need owner

[Daniel Erat, 2012-09-04 21:51:58]

https://chromiumcodereview.appspot.com/10907037/diff/1/ash/system/power/power...
File ash/system/power/power_status_view.cc (right):

https://chromiumcodereview.appspot.com/10907037/diff/1/ash/system/power/power...
ash/system/power/power_status_view.cc:152:
time_status_label_->SetText(time_status_text);
This seems like a worthwhile optimization to do elsewhere.  Any reason it can't
be placed in Views instead?

https://chromiumcodereview.appspot.com/10907037/diff/1/ash/system/power/power...
File ash/system/power/power_status_view.h (right):

https://chromiumcodereview.appspot.com/10907037/diff/1/ash/system/power/power...
ash/system/power/power_status_view.h:58: int battery_image_index_;
nit: add a comment describing what this is an index into

https://chromiumcodereview.appspot.com/10907037/diff/1/ash/system/power/tray_...
File ash/system/power/tray_power.h (right):

https://chromiumcodereview.appspot.com/10907037/diff/1/ash/system/power/tray_...
ash/system/power/tray_power.h:36: static int GetBatteryImageIndex(const
PowerSupplyStatus& supply_status);
nit: add comments documenting what these do

[sheu, 2012-09-11 20:28:18]

New patchset up.

[Daniel Erat, 2012-09-11 22:17:09]

https://chromiumcodereview.appspot.com/10907037/diff/8002/ash/system/power/po...
File ash/system/power/power_status_view.cc (right):

https://chromiumcodereview.appspot.com/10907037/diff/8002/ash/system/power/po...
ash/system/power/power_status_view.cc:209: if (icon_image_index_ == -1)
use curly-brackets for multi-line if statements

https://chromiumcodereview.appspot.com/10907037/diff/8002/ash/system/power/po...
ash/system/power/power_status_view.cc:210: // Default in case supply_status_ is
uncertain
nit: add period at end of comment.

https://chromiumcodereview.appspot.com/10907037/diff/8002/ash/system/power/po...
ash/system/power/power_status_view.cc:211: icon_->SetImage(icon_->GetImage());
this call seems strange...

https://chromiumcodereview.appspot.com/10907037/diff/8002/ash/system/power/po...
File ash/system/power/power_status_view.h (right):

https://chromiumcodereview.appspot.com/10907037/diff/8002/ash/system/power/po...
ash/system/power/power_status_view.h:57: // battery status indicator icon
nit: capitalize first letter and add period, e.g. "Battery status indicator
icon."

https://chromiumcodereview.appspot.com/10907037/diff/8002/ash/system/power/po...
ash/system/power/power_status_view.h:59: // index of the current icon in the
icon array image
nit: add blank line above this one

also, change comment:

// Index of the current icon in the icon array image, or
// -1 if the current status is unknown.

https://chromiumcodereview.appspot.com/10907037/diff/8002/ash/system/power/tr...
File ash/system/power/tray_power.cc (right):

https://chromiumcodereview.appspot.com/10907037/diff/8002/ash/system/power/tr...
ash/system/power/tray_power.cc:67: : battery_icon_index_(-1) {
nit: move to end of previous line

https://chromiumcodereview.appspot.com/10907037/diff/8002/ash/system/power/tr...
ash/system/power/tray_power.cc:90: SetImage(GetImage());
don't understand this either

https://chromiumcodereview.appspot.com/10907037/diff/8002/ash/system/power/tr...
ash/system/power/tray_power.cc:98: // index of the current icon in the icon
array image
nit: capitalize, add period, and mention what -1 means

https://chromiumcodereview.appspot.com/10907037/diff/8002/ash/system/power/tr...
ash/system/power/tray_power.cc:118: if (battery_icon_index_ == -1)
nit: add curly brackets

https://chromiumcodereview.appspot.com/10907037/diff/8002/ash/system/power/tr...
ash/system/power/tray_power.cc:119: SetIconImage(GetIconImage());
same as above

https://chromiumcodereview.appspot.com/10907037/diff/8002/ash/system/power/tr...
ash/system/power/tray_power.cc:130: // index of the current icon in the icon
array image
nit: capitalize and period

https://chromiumcodereview.appspot.com/10907037/diff/8002/ash/system/power/tr...
File ash/system/power/tray_power.h (right):

https://chromiumcodereview.appspot.com/10907037/diff/8002/ash/system/power/tr...
ash/system/power/tray_power.h:36: // Get the icon index in the battery icon
array image based on
s/Get/Gets/

https://chromiumcodereview.appspot.com/10907037/diff/8002/ash/system/power/tr...
ash/system/power/tray_power.h:38: // return -1.
s/return/returns/

https://chromiumcodereview.appspot.com/10907037/diff/8002/ash/system/power/tr...
ash/system/power/tray_power.h:40: // Lookup the actual icon in the icon array
image for |image_index|.
add a blank line before this one

s/Lookup/Looks up/

[sheu, 2012-09-11 23:08:55]

Up.

[Daniel Erat, 2012-09-11 23:31:04]

https://chromiumcodereview.appspot.com/10907037/diff/1006/ash/system/power/po...
File ash/system/power/power_status_view.cc (right):

https://chromiumcodereview.appspot.com/10907037/diff/1006/ash/system/power/po...
ash/system/power/power_status_view.cc:211: icon_->SetImage(icon_->GetImage());
didn't see any explanation for this: it looks like you're telling the icon to
use the same image that it's already using.  if this has some other side effect,
add a comment explaining what it is.

same question goes for two places in tray_power.cc.

https://chromiumcodereview.appspot.com/10907037/diff/1006/ash/system/power/po...
File ash/system/power/power_status_view.h (right):

https://chromiumcodereview.appspot.com/10907037/diff/1006/ash/system/power/po...
ash/system/power/power_status_view.h:60: // Index of the current icon in the
icon array image, or -1 if unknown
nit: add trailing period

[sheu, 2012-09-12 04:01:28]

The GetImage(), etc. strangeness was because the existing code did that and I
just sort of robotically copied that functionality.  Looking at the code itself,
it turns out we don't really need it., so I've removed 'em.

[Daniel Erat, 2012-09-12 04:06:18]

lgtm

Thanks!

[commit-bot: I haz the power, 2012-09-12 19:51:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/10907037/12003

[commit-bot: I haz the power, 2012-09-12 20:53:26]

Try job failure for 10907037-12003 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-09-13 00:00:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/10907037/12003

[commit-bot: I haz the power, 2012-09-13 02:42:31]

Change committed as 156474