chromeos: Simplify power supply info in PowerLibrary

chromeos: Simplify power supply info in PowerLibrary

Factoring out the status_ field of PowerLibrary, along with the numerous
accessor functions.  Instead, the power status will be used as a local
variable, passed along to observers as an argument.

Also, created a local version of the power supply status struct.  This is the
struct that will be used when the libcros version of the struct is removed.

See satorux's comments here:
http://codereview.chromium.org/8271024/

BUG=chromium-os:16558
TEST=Make sure power button icon behaves properly, on both AC and battery.

Signed-off-by: Simon Que <sque@chromium.org>;

R=satorux@chromium.org,stevenjb@chromium.org


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

[stevenjb, 2011-10-18 23:08:23]

http://codereview.chromium.org/8347016/diff/1001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/8347016/diff/1001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/power_library.cc:233: 180 -
status.battery_time_to_empty);
It seems like PowerStatus should just store percentage_ and have accessors for
TimeToEmpty and TimeToFull (or a single setter that sets all three).

http://codereview.chromium.org/8347016/diff/1001/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/power_menu_button.h (right):

http://codereview.chromium.org/8347016/diff/1001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/power_menu_button.h:11: #include
"third_party/cros/chromeos_power.h"
We shouldn't use the PowerStatus struct defined in libcros; that should be going
away. We should define a struct in Chrome instead.

[Simon Que, 2011-10-18 23:17:28]

http://codereview.chromium.org/8347016/diff/1001/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/power_menu_button.h (right):

http://codereview.chromium.org/8347016/diff/1001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/power_menu_button.h:11: #include
"third_party/cros/chromeos_power.h"
On 2011/10/18 23:08:23, Steven Bennetts wrote:
> We shouldn't use the PowerStatus struct defined in libcros; that should be
going
> away. We should define a struct in Chrome instead.

Agreed, but I was hoping to do it after this patch and the other one:
http://codereview.chromium.org/8347016/

[satorux1, 2011-10-19 21:16:55]

LGTM. Simon, thank you for the refactoring. It's great to see many getters to be
gone.

I tried to run try jobs for you, but git cl patch failed. Please updload the
rebased version.

http://codereview.chromium.org/8347016/diff/1001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/8347016/diff/1001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/power_library.cc:233: 180 -
status.battery_time_to_empty);
On 2011/10/18 23:08:23, Steven Bennetts wrote:
> It seems like PowerStatus should just store percentage_ and have accessors for
> TimeToEmpty and TimeToFull (or a single setter that sets all three).

This is a stub implementation.

PowerStatus data comes from power_manager, so we probably don't need to compute
them in Chrome.

[satorux1, 2011-10-19 21:35:05]

Started try jobs. The results probably won't appear here as I'm not the owner of
this code review, but I'll take care of the submission if the results are good.

[satorux1, 2011-10-20 04:32:39]

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.h:16: #include
"chrome/browser/chromeos/cros/power_library.h"
Hmm, testing_automation_provider.h seems to be shared by all platforms? If so,
chromeos specific code should be placed inside #if defined(OS_CHROMEOS)

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/power_library.cc:116: PowerSupplyStatus status;
You can zero-clear the struct by:

PowerSupplyStatus status = {};  // Zero-clear

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/power_library.cc:142: << " ttf=" <<
status.battery_time_to_full;
Would be nice to clean up names here. per= -> percentage= etc.

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/power_library.h (right):

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/power_library.h:28: int64 battery_time_to_empty;
Maybe  battery_time_to_empty_in_secs would be clearer.

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/chromeos/low_...
File chrome/browser/chromeos/low_battery_observer.cc (right):

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/chromeos/low_...
chrome/browser/chromeos/low_battery_observer.cc:11: #include
"third_party/cros/chromeos_power.h"
I thought we don't need this?

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/chromeos/low_...
chrome/browser/chromeos/low_battery_observer.cc:34: int remaining =
power_status.battery_time_to_empty;
What happend to |remaining_minutes| in the orignal code?

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/chromeos/low_...
chrome/browser/chromeos/low_battery_observer.cc:46: (notification_.urgent() ||
remaining <= critical);
This looks wrong. The orignal code was using remaining_minutes but |remaining|
is in seconds, right?

[Simon Que, 2011-10-20 21:18:53]

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.h:16: #include
"chrome/browser/chromeos/cros/power_library.h"
On 2011/10/20 04:32:40, satorux1 wrote:
> Hmm, testing_automation_provider.h seems to be shared by all platforms? If so,
> chromeos specific code should be placed inside #if defined(OS_CHROMEOS)

Done.

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/power_library.cc:116: PowerSupplyStatus status;
On 2011/10/20 04:32:40, satorux1 wrote:
> You can zero-clear the struct by:
> 
> PowerSupplyStatus status = {};  // Zero-clear

Done.

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/power_library.cc:142: << " ttf=" <<
status.battery_time_to_full;
On 2011/10/20 04:32:40, satorux1 wrote:
> Would be nice to clean up names here. per= -> percentage= etc. 

Done.

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/power_library.h (right):

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/power_library.h:28: int64 battery_time_to_empty;
On 2011/10/20 04:32:40, satorux1 wrote:
> Maybe  battery_time_to_empty_in_secs would be clearer.

Done.

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/chromeos/low_...
File chrome/browser/chromeos/low_battery_observer.cc (right):

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/chromeos/low_...
chrome/browser/chromeos/low_battery_observer.cc:11: #include
"third_party/cros/chromeos_power.h"
On 2011/10/20 04:32:40, satorux1 wrote:
> I thought we don't need this?

Done.

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/chromeos/low_...
chrome/browser/chromeos/low_battery_observer.cc:34: int remaining =
power_status.battery_time_to_empty;
On 2011/10/20 04:32:40, satorux1 wrote:
> What happend to |remaining_minutes| in the orignal code?

Fixed. Everything is a TimeDelta now.

http://codereview.chromium.org/8347016/diff/8001/chrome/browser/chromeos/low_...
chrome/browser/chromeos/low_battery_observer.cc:46: (notification_.urgent() ||
remaining <= critical);
On 2011/10/20 04:32:40, satorux1 wrote:
> This looks wrong. The orignal code was using remaining_minutes but |remaining|
> is in seconds, right?

Done.

[satorux1, 2011-10-21 19:36:49]

http://codereview.chromium.org/8347016/diff/12001/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/8347016/diff/12001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:45: public
chromeos::PowerLibrary::Observer {
This needs to be #if defined(OS_CHROMEOS). I'd suggest to have this before
public "content::NotificationObserver", that'll make it a bit simpler.

http://codereview.chromium.org/8347016/diff/12001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:56: virtual void
PowerChanged(const chromeos::PowerSupplyStatus& status) OVERRIDE;
Add this.

// chromeos::PowerLibrary::Observer overrides.

http://codereview.chromium.org/8347016/diff/12001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:57: virtual void
SystemResumed() OVERRIDE {}
Ditto. This needs to be in #if defined(OS_CHROMEOS). Move this to below where
#if defined(OS_CHROMEOS) is already present.

[Simon Que, 2011-10-21 20:55:31]

http://codereview.chromium.org/8347016/diff/12001/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/8347016/diff/12001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:45: public
chromeos::PowerLibrary::Observer {
On 2011/10/21 19:36:50, satorux1 wrote:
> This needs to be #if defined(OS_CHROMEOS). I'd suggest to have this before
> public "content::NotificationObserver", that'll make it a bit simpler.

Done.

http://codereview.chromium.org/8347016/diff/12001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:56: virtual void
PowerChanged(const chromeos::PowerSupplyStatus& status) OVERRIDE;
On 2011/10/21 19:36:50, satorux1 wrote:
> Add this.
> 
> // chromeos::PowerLibrary::Observer overrides.

Done.

http://codereview.chromium.org/8347016/diff/12001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:57: virtual void
SystemResumed() OVERRIDE {}
On 2011/10/21 19:36:50, satorux1 wrote:
> Ditto. This needs to be in #if defined(OS_CHROMEOS). Move this to below where
> #if defined(OS_CHROMEOS) is already present.

Done

[stevenjb, 2011-10-24 20:36:21]

http://codereview.chromium.org/8347016/diff/18003/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/8347016/diff/18003/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:45: public
chromeos::PowerLibrary::Observer,
We should really have a TestingAutomationProviderChromeos class instead of all
these #ifdefs.

At minimum, we should implement a PowerLibrary::Observer class inside the
_chromeos .cc file and forward declare the class here instead of introducing a
conditional include + parent class. Then we can just add a
scoped_ptr<TestingAutomationPowerLibraryObserver> member here.

http://codereview.chromium.org/8347016/diff/18003/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:1429: #endif  //
defined(OS_CHROMEOS)
We should just make this a static variable in the .cc file instead of adding it
to the shared header.

http://codereview.chromium.org/8347016/diff/18003/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/8347016/diff/18003/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:113: power_status) {
nit: put 'void* object' on its own line so that power_status line doesn't need
to be broken up

[Simon Que, 2011-10-24 22:49:48]

PTAL

http://codereview.chromium.org/8347016/diff/18003/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/8347016/diff/18003/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:45: public
chromeos::PowerLibrary::Observer,
On 2011/10/24 20:36:21, Steven Bennetts wrote:
> We should really have a TestingAutomationProviderChromeos class instead of all
> these #ifdefs.
> 
> At minimum, we should implement a PowerLibrary::Observer class inside the
> _chromeos .cc file and forward declare the class here instead of introducing a
> conditional include + parent class. Then we can just add a
> scoped_ptr<TestingAutomationPowerLibraryObserver> member here.

Can this be done in a later patch?

http://codereview.chromium.org/8347016/diff/18003/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:1429: #endif  //
defined(OS_CHROMEOS)
On 2011/10/24 20:36:21, Steven Bennetts wrote:
> We should just make this a static variable in the .cc file instead of adding
it
> to the shared header.

Done.

http://codereview.chromium.org/8347016/diff/18003/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/8347016/diff/18003/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:113: power_status) {
On 2011/10/24 20:36:21, Steven Bennetts wrote:
> nit: put 'void* object' on its own line so that power_status line doesn't need
> to be broken up

Done.

[stevenjb, 2011-10-25 02:33:19]

http://codereview.chromium.org/8347016/diff/18003/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/8347016/diff/18003/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:45: public
chromeos::PowerLibrary::Observer,
On 2011/10/24 22:49:48, Simon Que wrote:
> On 2011/10/24 20:36:21, Steven Bennetts wrote:
> > We should really have a TestingAutomationProviderChromeos class instead of
all
> > these #ifdefs.
> > 
> > At minimum, we should implement a PowerLibrary::Observer class inside the
> > _chromeos .cc file and forward declare the class here instead of introducing
a
> > conditional include + parent class. Then we can just add a
> > scoped_ptr<TestingAutomationPowerLibraryObserver> member here.
> 
> Can this be done in a later patch?

I am reluctant to introduce new #ifdefs into such a key piece of code; adding
the Observer as a member is a small enough change that I really think it is
worth doing instead. Also, in general adding a member is preferable to multiple
inheritance (even though we do it frequently with observers). I'd rather change
that pattern for TestingAutomationProvider sooner rather than later.
(I do agree that creating TestingAutomationProviderChromeos in this patch
doesn't make any sense).

http://codereview.chromium.org/8347016/diff/25001/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/8347016/diff/25001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider_chromeos.cc:219: }
Since this observer is only changing a local variable (i.e. we don't even need
to add an accessor to TestingAutomationProvider) it will be simple to keep the
Observer implementation here instead of in the header.

[Simon Que, 2011-10-25 19:30:47]

http://codereview.chromium.org/8347016/diff/18003/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/8347016/diff/18003/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:45: public
chromeos::PowerLibrary::Observer,
On 2011/10/25 02:33:19, Steven Bennetts wrote:
> On 2011/10/24 22:49:48, Simon Que wrote:
> > On 2011/10/24 20:36:21, Steven Bennetts wrote:
> > > We should really have a TestingAutomationProviderChromeos class instead of
> all
> > > these #ifdefs.
> > > 
> > > At minimum, we should implement a PowerLibrary::Observer class inside the
> > > _chromeos .cc file and forward declare the class here instead of
introducing
> a
> > > conditional include + parent class. Then we can just add a
> > > scoped_ptr<TestingAutomationPowerLibraryObserver> member here.
> > 
> > Can this be done in a later patch?
> 
> I am reluctant to introduce new #ifdefs into such a key piece of code; adding
> the Observer as a member is a small enough change that I really think it is
> worth doing instead. Also, in general adding a member is preferable to
multiple
> inheritance (even though we do it frequently with observers). I'd rather
change
> that pattern for TestingAutomationProvider sooner rather than later.
> (I do agree that creating TestingAutomationProviderChromeos in this patch
> doesn't make any sense). 

I'm having a bit of trouble with this due to the complex dependencies.  Let's
talk about this over chat.

[stevenjb, 2011-10-25 21:41:49]

lgtm with nits

http://codereview.chromium.org/8347016/diff/28001/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/8347016/diff/28001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:47: // source file.
nit: I created crosbug.com/22081, we should make this:
// TODO(sque): Move this to a ChromeOS specific subclass. See crosbug.com/22081

Also, I'd put the comment with the first #define instead (plus it will fit on
one line there).

http://codereview.chromium.org/8347016/diff/28001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/8347016/diff/28001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:230: PowerSupplyStatus status =
{};
nit: = {} is unnecessary

http://codereview.chromium.org/8347016/diff/28001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/power_library.h (right):

http://codereview.chromium.org/8347016/diff/28001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.h:27: // Time in seconds until the
battery is considered empty, 0 for unknown.
nit: s/empty/empty or full/

[satorux1, 2011-10-25 21:43:28]

LGTM. running try jobs...

http://codereview.chromium.org/8347016/diff/28001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/8347016/diff/28001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:230: PowerSupplyStatus status =
{};
On 2011/10/25 21:41:49, Steven Bennetts wrote:
> nit: = {} is unnecessary

{} is for zero-clear the struct.

[stevenjb, 2011-10-25 21:54:20]

http://codereview.chromium.org/8347016/diff/28001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/8347016/diff/28001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:230: PowerSupplyStatus status =
{};
On 2011/10/25 21:43:28, satorux1 wrote:
> On 2011/10/25 21:41:49, Steven Bennetts wrote:
> > nit: = {} is unnecessary
> 
> {} is for zero-clear the struct.

Ah, you're right, I forgot about that C++ wonkiness. Might want to add a comment
to that effect.

[Simon Que, 2011-10-25 22:59:16]

http://codereview.chromium.org/8347016/diff/28001/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/8347016/diff/28001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:47: // source file.
On 2011/10/25 21:41:49, Steven Bennetts wrote:
> nit: I created crosbug.com/22081, we should make this:
> // TODO(sque): Move this to a ChromeOS specific subclass. See
crosbug.com/22081
> 
> Also, I'd put the comment with the first #define instead (plus it will fit on
> one line there).

Done.

http://codereview.chromium.org/8347016/diff/28001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/8347016/diff/28001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:230: PowerSupplyStatus status =
{};
On 2011/10/25 21:54:20, Steven Bennetts wrote:
> On 2011/10/25 21:43:28, satorux1 wrote:
> > On 2011/10/25 21:41:49, Steven Bennetts wrote:
> > > nit: = {} is unnecessary
> > 
> > {} is for zero-clear the struct.
> 
> Ah, you're right, I forgot about that C++ wonkiness. Might want to add a
comment
> to that effect. 

Done.

http://codereview.chromium.org/8347016/diff/28001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/power_library.h (right):

http://codereview.chromium.org/8347016/diff/28001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.h:27: // Time in seconds until the
battery is considered empty, 0 for unknown.
On 2011/10/25 21:41:49, Steven Bennetts wrote:
> nit: s/empty/empty or full/

Done.

[satorux1, 2011-10-26 07:03:13]

LGTM. try results were positive. Should be ready to commit.

try success for sque#f3fa7 on linux_chromeos @ r107168

[commit-bot: I haz the power, 2011-10-26 22:15:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sque@chromium.org/8347016/27003

[commit-bot: I haz the power, 2011-10-26 23:27:14]

Change committed as 107465