Add battery status to settings row in uber tray bubble.

Add battery status to settings row in uber tray bubble.


BUG=127771
TEST=power status showing on settings row if the battery presents.

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

[stevenjb, 2012-06-07 01:31:18]

http://codereview.chromium.org/10540041/diff/1/ash/system/power/power_status_...
File ash/system/power/power_status_view.cc (right):

http://codereview.chromium.org/10540041/diff/1/ash/system/power/power_status_...
ash/system/power/power_status_view.cc:43: }
I realize that I set the precedent, but I now that we have split this into a
separate file, I wonder if we wouldn't be better off separating the text part
into its own internal view? That would remove a bunch of private functions and
members from the header, and maybe make this a little more readable? Not
critical, only a suggestion.

http://codereview.chromium.org/10540041/diff/1/ash/system/power/power_status_...
ash/system/power/power_status_view.cc:59: 3 /* padding between
time_status_label_ and icon_ */
nit: This should really be a const variable with the comment at the declaration

http://codereview.chromium.org/10540041/diff/1/ash/system/power/power_status_...
File ash/system/power/power_status_view.h (right):

http://codereview.chromium.org/10540041/diff/1/ash/system/power/power_status_...
ash/system/power/power_status_view.h:19: namespace tray {
I don't think we should use namespace tray for anything exposed in a header;
internal seems sufficient.

http://codereview.chromium.org/10540041/diff/1/ash/system/power/tray_power.h
File ash/system/power/tray_power.h (right):

http://codereview.chromium.org/10540041/diff/1/ash/system/power/tray_power.h#...
ash/system/power/tray_power.h:68: IconSet icon_set);
nit: align

http://codereview.chromium.org/10540041/diff/1/ash/system/power/tray_power.h#...
ash/system/power/tray_power.h:68: IconSet icon_set);
Also, this would be better as a static member function of TrayPower than a
global (even if it is namespaced - ash::internal is actually a pretty large
namespace).

http://codereview.chromium.org/10540041/diff/1/ash/system/settings/tray_setti...
File ash/system/settings/tray_settings.cc (right):

http://codereview.chromium.org/10540041/diff/1/ash/system/settings/tray_setti...
ash/system/settings/tray_settings.cc:59:
power_status_view_->UpdatePowerStatus(power_status);
nit: Put this into UpdatePowerStatus()? (See comment below)

http://codereview.chromium.org/10540041/diff/1/ash/system/settings/tray_setti...
ash/system/settings/tray_settings.cc:147:
default_->power_status_view()->UpdatePowerStatus(status);
nit: Make UpdatePowerStatus() a member of PowerStatusView instead of exposing
power_status_view()?

http://codereview.chromium.org/10540041/diff/1/ash/system/settings/tray_setti...
File ash/system/settings/tray_settings.h (right):

http://codereview.chromium.org/10540041/diff/1/ash/system/settings/tray_setti...
ash/system/settings/tray_settings.h:38: tray::SettingsDefaultView* default_;
nit: default_view_?

http://codereview.chromium.org/10540041/diff/1/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10540041/diff/1/ash/system/tray/system_tray.cc...
ash/system/tray/system_tray.cc:196: power_status_observer_ = tray_power;
Remove this?

http://codereview.chromium.org/10540041/diff/1/ash/system/tray/system_tray.cc...
ash/system/tray/system_tray.cc:305: {
nit: Put arg + { on second line

http://codereview.chromium.org/10540041/diff/1/ash/system/tray/system_tray.h
File ash/system/tray/system_tray.h (right):

http://codereview.chromium.org/10540041/diff/1/ash/system/tray/system_tray.h#...
ash/system/tray/system_tray.h:228: std::vector<PowerStatusObserver*>
power_status_observers_;
Do we need both power_status_observer_ and power_status_observers_?
Also, for consistency with other code, I think we probably want to use
ObserverList and FOR_EACH_OBSERVER.

[sadrul, 2012-06-07 15:14:35]

http://codereview.chromium.org/10540041/diff/1/ash/system/power/power_status_...
File ash/system/power/power_status_view.cc (right):

http://codereview.chromium.org/10540041/diff/1/ash/system/power/power_status_...
ash/system/power/power_status_view.cc:33:
PowerStatusView::PowerStatusView(ViewType view_type) :
: in the next line

http://codereview.chromium.org/10540041/diff/1/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10540041/diff/1/ash/system/tray/system_tray.cc...
ash/system/tray/system_tray.cc:196: power_status_observer_ = tray_power;
On 2012/06/07 01:31:18, stevenjb (chromium) wrote:
> Remove this?

A bit ugly, but a slightly different approach: make SystemTray itself a
PowerStatusObserver, and update tray_power and tray_settings from the
SystemTray::OnPowerStatusChanged callback. So you wouldn't need the change in
chromeos system-tray-delegate

Or: just have ObserverList<PowerStatusObserver> power_status_observers_, and
expose that to chromeos-system-tray-delegate. The delegate can then
FOR_EACH_OBSERVER on the list. This is probably better.

[jennyz, 2012-06-07 22:06:51]

One more issue I found when testing the patch on lumpy: When user switch between
log in status, for example, sign in as a guest user or owner, sign out, then
click the uber tray bubble, the battery time can be insane number. I did some
debug and trace the problem to SystemTrayDelegate::PowerChanged(), the new
PowerSupplyStatus object contains garbage data(some very big insane number) in
averaged_battery_time_to_empty, averaged_battery_time_to_full,
battery_seconds_to_empty, battery_seconds_to_full. This looks like a bug in crOS
battery status code, I didn't do santity check in our rendering code, which
would cover the problem if I did so. I would like to leave the rendering code as
it, show the problem, and file a bug against crOS. Would this approach be OK to
you guys?

http://codereview.chromium.org/10540041/diff/1/ash/system/power/power_status_...
File ash/system/power/power_status_view.cc (right):

http://codereview.chromium.org/10540041/diff/1/ash/system/power/power_status_...
ash/system/power/power_status_view.cc:33:
PowerStatusView::PowerStatusView(ViewType view_type) :
On 2012/06/07 15:14:35, sadrul wrote:
> : in the next line

Done.

http://codereview.chromium.org/10540041/diff/1/ash/system/power/power_status_...
ash/system/power/power_status_view.cc:43: }
On 2012/06/07 01:31:18, stevenjb (chromium) wrote:
> I realize that I set the precedent, but I now that we have split this into a
> separate file, I wonder if we wouldn't be better off separating the text part
> into its own internal view? That would remove a bunch of private functions and
> members from the header, and maybe make this a little more readable? Not
> critical, only a suggestion.
Yes, I feel this is a bit unclean. Maybe it is better to split it into
PowerStatusDefaultView and PowerStatusNotificationView? Can I do it in a
separate cl later?

http://codereview.chromium.org/10540041/diff/1/ash/system/power/power_status_...
ash/system/power/power_status_view.cc:59: 3 /* padding between
time_status_label_ and icon_ */
On 2012/06/07 01:31:18, stevenjb (chromium) wrote:
> nit: This should really be a const variable with the comment at the
declaration

Done.

http://codereview.chromium.org/10540041/diff/1/ash/system/power/power_status_...
File ash/system/power/power_status_view.h (right):

http://codereview.chromium.org/10540041/diff/1/ash/system/power/power_status_...
ash/system/power/power_status_view.h:19: namespace tray {
On 2012/06/07 01:31:18, stevenjb (chromium) wrote:
> I don't think we should use namespace tray for anything exposed in a header;
> internal seems sufficient.

Done.

http://codereview.chromium.org/10540041/diff/1/ash/system/power/tray_power.h
File ash/system/power/tray_power.h (right):

http://codereview.chromium.org/10540041/diff/1/ash/system/power/tray_power.h#...
ash/system/power/tray_power.h:68: IconSet icon_set);
On 2012/06/07 01:31:18, stevenjb (chromium) wrote:
> nit: align

Done.

http://codereview.chromium.org/10540041/diff/1/ash/system/power/tray_power.h#...
ash/system/power/tray_power.h:68: IconSet icon_set);
On 2012/06/07 01:31:18, stevenjb (chromium) wrote:
> Also, this would be better as a static member function of TrayPower than a
> global (even if it is namespaced - ash::internal is actually a pretty large
> namespace).

Done.

http://codereview.chromium.org/10540041/diff/1/ash/system/settings/tray_setti...
File ash/system/settings/tray_settings.cc (right):

http://codereview.chromium.org/10540041/diff/1/ash/system/settings/tray_setti...
ash/system/settings/tray_settings.cc:59:
power_status_view_->UpdatePowerStatus(power_status);
On 2012/06/07 01:31:18, stevenjb (chromium) wrote:
> nit: Put this into UpdatePowerStatus()? (See comment below)

Done.

http://codereview.chromium.org/10540041/diff/1/ash/system/settings/tray_setti...
ash/system/settings/tray_settings.cc:147:
default_->power_status_view()->UpdatePowerStatus(status);
On 2012/06/07 01:31:18, stevenjb (chromium) wrote:
> nit: Make UpdatePowerStatus() a member of PowerStatusView instead of exposing
> power_status_view()?
Done, you mean make UpdatePowerStatus() a member of SettingsDefaultView, right?

http://codereview.chromium.org/10540041/diff/1/ash/system/settings/tray_setti...
File ash/system/settings/tray_settings.h (right):

http://codereview.chromium.org/10540041/diff/1/ash/system/settings/tray_setti...
ash/system/settings/tray_settings.h:38: tray::SettingsDefaultView* default_;
On 2012/06/07 01:31:18, stevenjb (chromium) wrote:
> nit: default_view_?

Done.

http://codereview.chromium.org/10540041/diff/1/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10540041/diff/1/ash/system/tray/system_tray.cc...
ash/system/tray/system_tray.cc:196: power_status_observer_ = tray_power;
On 2012/06/07 01:31:18, stevenjb (chromium) wrote:
> Remove this?

Done.

http://codereview.chromium.org/10540041/diff/1/ash/system/tray/system_tray.cc...
ash/system/tray/system_tray.cc:196: power_status_observer_ = tray_power;
On 2012/06/07 15:14:35, sadrul wrote:
> On 2012/06/07 01:31:18, stevenjb (chromium) wrote:
> > Remove this?
> 
> A bit ugly, but a slightly different approach: make SystemTray itself a
> PowerStatusObserver, and update tray_power and tray_settings from the
> SystemTray::OnPowerStatusChanged callback. So you wouldn't need the change in
> chromeos system-tray-delegate
> 
> Or: just have ObserverList<PowerStatusObserver> power_status_observers_, and
> expose that to chromeos-system-tray-delegate. The delegate can then
> FOR_EACH_OBSERVER on the list. This is probably better.

Done.

http://codereview.chromium.org/10540041/diff/1/ash/system/tray/system_tray.cc...
ash/system/tray/system_tray.cc:305: {
On 2012/06/07 01:31:18, stevenjb (chromium) wrote:
> nit: Put arg + { on second line

Done.

http://codereview.chromium.org/10540041/diff/1/ash/system/tray/system_tray.h
File ash/system/tray/system_tray.h (right):

http://codereview.chromium.org/10540041/diff/1/ash/system/tray/system_tray.h#...
ash/system/tray/system_tray.h:228: std::vector<PowerStatusObserver*>
power_status_observers_;
On 2012/06/07 01:31:18, stevenjb (chromium) wrote:
> Do we need both power_status_observer_ and power_status_observers_?
> Also, for consistency with other code, I think we probably want to use
> ObserverList and FOR_EACH_OBSERVER.

Done.

[jennyz, 2012-06-07 22:25:57]

By the way, I filed crosbug.com/31633 against the garbage PowerSupplyStatus data
issue.

[jennyz, 2012-06-07 23:20:19]

I found an issue of clicking settings row in uber tray when user is not logged
in or screen is locked. Fixed it in patch #3.

[sadrul, 2012-06-08 13:22:39]

LGTM with the nits addressed

http://codereview.chromium.org/10540041/diff/10004/ash/system/power/power_sta...
File ash/system/power/power_status_view.cc (right):

http://codereview.chromium.org/10540041/diff/10004/ash/system/power/power_sta...
ash/system/power/power_status_view.cc:34: view_type_(view_type) {
Initialize all the labels here to NULL

http://codereview.chromium.org/10540041/diff/10004/ash/system/power/power_sta...
ash/system/power/power_status_view.cc:85:
supply_status_.averaged_battery_time_to_empty);
You cannot rely on |time| here giving you the right value if
is_calculating_battery_time is set. So check for that first. (here and for
notification view) [this is probably an issue in the existing code too]

http://codereview.chromium.org/10540041/diff/10004/ash/system/power/power_sta...
ash/system/power/power_status_view.cc:89: if (supply_status_.line_power_on &&
!hour && !min) {
Is checking for .line_power_on && .battery_is_full sufficient here?

http://codereview.chromium.org/10540041/diff/10004/ash/system/power/power_sta...
File ash/system/power/power_status_view.h (right):

http://codereview.chromium.org/10540041/diff/10004/ash/system/power/power_sta...
ash/system/power/power_status_view.h:24: VIEW_DEFAULT,
too much indentation here

[jennyz, 2012-06-08 16:48:03]

http://codereview.chromium.org/10540041/diff/10004/ash/system/power/power_sta...
File ash/system/power/power_status_view.cc (right):

http://codereview.chromium.org/10540041/diff/10004/ash/system/power/power_sta...
ash/system/power/power_status_view.cc:34: view_type_(view_type) {
On 2012/06/08 13:22:39, sadrul wrote:
> Initialize all the labels here to NULL

Done.

http://codereview.chromium.org/10540041/diff/10004/ash/system/power/power_sta...
ash/system/power/power_status_view.cc:85:
supply_status_.averaged_battery_time_to_empty);
On 2012/06/08 13:22:39, sadrul wrote:
> You cannot rely on |time| here giving you the right value if
> is_calculating_battery_time is set. So check for that first. (here and for
> notification view) [this is probably an issue in the existing code too]

Done.

http://codereview.chromium.org/10540041/diff/10004/ash/system/power/power_sta...
ash/system/power/power_status_view.cc:89: if (supply_status_.line_power_on &&
!hour && !min) {
On 2012/06/08 13:22:39, sadrul wrote:
> Is checking for .line_power_on && .battery_is_full sufficient here?

Done.

http://codereview.chromium.org/10540041/diff/10004/ash/system/power/power_sta...
File ash/system/power/power_status_view.h (right):

http://codereview.chromium.org/10540041/diff/10004/ash/system/power/power_sta...
ash/system/power/power_status_view.h:24: VIEW_DEFAULT,
On 2012/06/08 13:22:39, sadrul wrote:
> too much indentation here

Done.

[stevenjb (google-dont-use), 2012-06-08 17:11:16]

lgtm w/ nit

http://codereview.chromium.org/10540041/diff/1013/ash/system/power/power_stat...
File ash/system/power/power_status_view.h (right):

http://codereview.chromium.org/10540041/diff/1013/ash/system/power/power_stat...
ash/system/power/power_status_view.h:20: // This view is used only for the
popup.
This comment no longer applies. We should change it and mention that it is used
by the power and settings tray.

[jennyz, 2012-06-08 17:29:51]

http://codereview.chromium.org/10540041/diff/1013/ash/system/power/power_stat...
File ash/system/power/power_status_view.h (right):

http://codereview.chromium.org/10540041/diff/1013/ash/system/power/power_stat...
ash/system/power/power_status_view.h:20: // This view is used only for the
popup.
On 2012/06/08 17:11:16, stevenjb1 wrote:
> This comment no longer applies. We should change it and mention that it is
used
> by the power and settings tray.

Done.

[commit-bot: I haz the power, 2012-06-08 17:47:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennyz@chromium.org/10540041/13

[commit-bot: I haz the power, 2012-06-08 17:47:54]

Presubmit check for 10540041-13 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ash

Presubmit checks took 1.3s to calculate.

[jennyz, 2012-06-08 17:51:24]

Add ash OWNER review.

[sky, 2012-06-08 17:57:56]

LGTM