chromeos: Add power supply info reading capability

chromeos: Add power supply info reading capability

Using chrome instead of libcros to request power supply status from
power manager.

BUG=chromium-os:16558
TEST=plug/unplug AC, make sure power icon updates correctly.

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

[satorux1, 2011-10-14 05:56:28]

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/chrome_...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/chrome_...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:76:
ChromeBrowserMainPartsChromeos::~ChromeBrowserMainPartsChromeos() {
Just like http://codereview.chromium.org/8271010/, please remove the observer
from >power_manager_client() like:

chromeos::DBusThreadManager::Get()->power_manager_client()->
    RemoveObserver(power_lib);

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/chrome_...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:136:
brightness_observer);
Sorry, I just submitted a patch to clean this up:
http://codereview.chromium.org/8271010/

Please sync your client and fix conflicts carefully.

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/chrome_...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:139: static
chromeos::PowerManagerClient::Observer* power_lib =
Please remove static.

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/cros/po...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/cros/po...
chrome/browser/chromeos/cros/power_library.cc:108: DVLOG(1) << "Power lpo=" <<
status.line_power_on
Please just use VLOG(1). DVLOG is rarely useful.

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/cros/po...
chrome/browser/chromeos/cros/power_library.cc:109: << " sta=" <<
status.battery_state
We eschew abbreviations like "sta" per our C++ style guide:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/cros/po...
chrome/browser/chromeos/cros/power_library.cc:112: << " ttf=" <<
status.battery_time_to_full;
I'd suggest to add ToString() function to PowerStatus, and change code to
something like

VLOG(1) << "PowerStatus: " << status.ToString();

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/cros/po...
chrome/browser/chromeos/cros/power_library.cc:114: FOR_EACH_OBSERVER(Observer,
observers_, PowerChanged(this));
This looks rather redundant.

UpdatePowerStatus() is called from PowerManagerClient via FOR_EACH_OBSERVER(),
and then again notifying other observers here by FOR_EACH_OBSERVER(). Would be
simpler to get rid of this layer.

However, the redundancy is currently necessary as you need to update |status_|
here. but I think you can remove |status_| by getting rid of all getters like 
battery_fully_charged(). Instead of exposing these getters, I think you can pass
|status| to PowerChanged() like PowerChanged(status) instead of
PowerChanged(this).

Seems to me that the observer is implemented in PowerMenuButton like

void PowerMenuButton::PowerChanged(PowerLibrary* obj) {
  UpdateIconAndLabelInfo();
}

and |obj| is unsed as you see. In UpdateIconAndLabelInfo(), getters like 
 line_power_on() are used, but these callers can be removed if you pass |status|
to UpdateIconAndLabelInfo().

I'd suggest to refactor code around |status_| as a separate patch and submit it
before this patch.

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/cros/po...
chrome/browser/chromeos/cros/power_library.cc:139: }
I think you can remove this function. No longer, used, right?

[satorux1, 2011-10-14 05:57:26]

As mentioned off-line, let's wait until M16 branch is cut on Tuesday, before
submitting this, as this patch is potentially risky, and we don't want to
introduce regressions at this moment. :)

[Simon Que, 2011-10-18 19:23:06]

PTAL

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/chrome_...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/chrome_...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:76:
ChromeBrowserMainPartsChromeos::~ChromeBrowserMainPartsChromeos() {
On 2011/10/14 05:56:28, satorux1 wrote:
> Just like http://codereview.chromium.org/8271010/, please remove the observer
> from >power_manager_client() like:
> 
> chromeos::DBusThreadManager::Get()->power_manager_client()->
>     RemoveObserver(power_lib);

Done.

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/chrome_...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:136:
brightness_observer);
On 2011/10/14 05:56:28, satorux1 wrote:
> Sorry, I just submitted a patch to clean this up:
> http://codereview.chromium.org/8271010/
> 
> Please sync your client and fix conflicts carefully.

Done.

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/chrome_...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:139: static
chromeos::PowerManagerClient::Observer* power_lib =
On 2011/10/14 05:56:28, satorux1 wrote:
> Please remove static.

Done.

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/cros/po...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/cros/po...
chrome/browser/chromeos/cros/power_library.cc:108: DVLOG(1) << "Power lpo=" <<
status.line_power_on
On 2011/10/14 05:56:28, satorux1 wrote:
> Please just use VLOG(1). DVLOG is rarely useful.

Since this is just being moved, can I make this change (and the next few
suggestions) in another patch?

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/cros/po...
chrome/browser/chromeos/cros/power_library.cc:139: }
On 2011/10/14 05:56:28, satorux1 wrote:
> I think you can remove this function. No longer, used, right?

Done.

[satorux1, 2011-10-18 19:41:54]

Some of my comments were not answered. What do you think about ToString()
and the idea of getting rid of getters to make the class simpler?

On Tue, Oct 18, 2011 at 12:23 PM, <sque@chromium.org> wrote:

> PTAL



> http://codereview.chromium.**org/8271024/diff/1/chrome/**
>
browser/chromeos/chrome_**browser_main_chromeos.cc<http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/chrome_browser_main_chromeos.cc>
> File chrome/browser/chromeos/**chrome_browser_main_chromeos.**cc (right):

> http://codereview.chromium.**org/8271024/diff/1/chrome/**
>
browser/chromeos/chrome_**browser_main_chromeos.cc#**newcode76<http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/chrome_browser_main_chromeos.cc#newcode76>
> chrome/browser/chromeos/**chrome_browser_main_chromeos.**cc:76:
> ChromeBrowserMainPartsChromeos**::~**ChromeBrowserMainPartsChromeos**() {
> On 2011/10/14 05:56:28, satorux1 wrote:

>> Just like  
>>
http://codereview.chromium.**org/8271010/<http://codereview.chromium.org/8271...,
>> please remove the

> observer

>> from >power_manager_client() like:


>   chromeos::DBusThreadManager::**Get()->power_manager_client()-**>
>>      RemoveObserver(power_lib);


> Done.


> http://codereview.chromium.**org/8271024/diff/1/chrome/**
>
browser/chromeos/chrome_**browser_main_chromeos.cc#**newcode136<http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/chrome_browser_main_chromeos.cc#newcode136>
> chrome/browser/chromeos/**chrome_browser_main_chromeos.**cc:136:
> brightness_observer);
> On 2011/10/14 05:56:28, satorux1 wrote:

>> Sorry, I just submitted a patch to clean this up:
>>
http://codereview.chromium.**org/8271010/<http://codereview.chromium.org/8271...


>   Please sync your client and fix conflicts carefully.


> Done.


> http://codereview.chromium.**org/8271024/diff/1/chrome/**
>
browser/chromeos/chrome_**browser_main_chromeos.cc#**newcode139<http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/chrome_browser_main_chromeos.cc#newcode139>
> chrome/browser/chromeos/**chrome_browser_main_chromeos.**cc:139: static
> chromeos::PowerManagerClient::**Observer* power_lib =
> On 2011/10/14 05:56:28, satorux1 wrote:

>> Please remove static.


> Done.


> http://codereview.chromium.**org/8271024/diff/1/chrome/**
>
browser/chromeos/cros/power_**library.cc<http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/cros/power_library.cc>
> File chrome/browser/chromeos/cros/**power_library.cc (right):

> http://codereview.chromium.**org/8271024/diff/1/chrome/**
>
browser/chromeos/cros/power_**library.cc#newcode108<http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/cros/power_library.cc#newcode108>
> chrome/browser/chromeos/cros/**power_library.cc:108: DVLOG(1) << "Power
> lpo=" << status.line_power_on
> On 2011/10/14 05:56:28, satorux1 wrote:

>> Please just use VLOG(1). DVLOG is rarely useful.


> Since this is just being moved, can I make this change (and the next few
> suggestions) in another patch?

> http://codereview.chromium.**org/8271024/diff/1/chrome/**
>
browser/chromeos/cros/power_**library.cc#newcode139<http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/cros/power_library.cc#newcode139>
> chrome/browser/chromeos/cros/**power_library.cc:139: }

> On 2011/10/14 05:56:28, satorux1 wrote:

>> I think you can remove this function. No longer, used, right?


> Done.

>
http://codereview.chromium.**org/8271024/<http://codereview.chromium.org/8271...

[Simon Que, 2011-10-18 21:09:34]

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/cros/po...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/cros/po...
chrome/browser/chromeos/cros/power_library.cc:114: FOR_EACH_OBSERVER(Observer,
observers_, PowerChanged(this));
On 2011/10/14 05:56:28, satorux1 wrote:
> This looks rather redundant.
> 
> UpdatePowerStatus() is called from PowerManagerClient via FOR_EACH_OBSERVER(),
> and then again notifying other observers here by FOR_EACH_OBSERVER(). Would be
> simpler to get rid of this layer.
> 
> However, the redundancy is currently necessary as you need to update |status_|
> here. but I think you can remove |status_| by getting rid of all getters like 
> battery_fully_charged(). Instead of exposing these getters, I think you can
pass
> |status| to PowerChanged() like PowerChanged(status) instead of
> PowerChanged(this).
> 
> Seems to me that the observer is implemented in PowerMenuButton like
> 
> void PowerMenuButton::PowerChanged(PowerLibrary* obj) {
>   UpdateIconAndLabelInfo();
> }
> 
> and |obj| is unsed as you see. In UpdateIconAndLabelInfo(), getters like 
>  line_power_on() are used, but these callers can be removed if you pass
|status|
> to UpdateIconAndLabelInfo().
> 
> I'd suggest to refactor code around |status_| as a separate patch and submit
it
> before this patch.

What about in
src/chrome/browser/automation/testing_automation_provider_chromeos.cc?  It still
depends on those Get() functions...

[satorux1, 2011-10-18 22:07:13]

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/cros/po...
File chrome/browser/chromeos/cros/power_library.cc (right):

http://codereview.chromium.org/8271024/diff/1/chrome/browser/chromeos/cros/po...
chrome/browser/chromeos/cros/power_library.cc:114: FOR_EACH_OBSERVER(Observer,
observers_, PowerChanged(this));
On 2011/10/18 21:09:34, Simon Que wrote:
> On 2011/10/14 05:56:28, satorux1 wrote:
> > This looks rather redundant.
> > 
> > UpdatePowerStatus() is called from PowerManagerClient via
FOR_EACH_OBSERVER(),
> > and then again notifying other observers here by FOR_EACH_OBSERVER(). Would
be
> > simpler to get rid of this layer.
> > 
> > However, the redundancy is currently necessary as you need to update
|status_|
> > here. but I think you can remove |status_| by getting rid of all getters
like 
> > battery_fully_charged(). Instead of exposing these getters, I think you can
> pass
> > |status| to PowerChanged() like PowerChanged(status) instead of
> > PowerChanged(this).
> > 
> > Seems to me that the observer is implemented in PowerMenuButton like
> > 
> > void PowerMenuButton::PowerChanged(PowerLibrary* obj) {
> >   UpdateIconAndLabelInfo();
> > }
> > 
> > and |obj| is unsed as you see. In UpdateIconAndLabelInfo(), getters like 
> >  line_power_on() are used, but these callers can be removed if you pass
> |status|
> > to UpdateIconAndLabelInfo().
> > 
> > I'd suggest to refactor code around |status_| as a separate patch and submit
> it
> > before this patch.
> 
> What about in
> src/chrome/browser/automation/testing_automation_provider_chromeos.cc?  It
still
> depends on those Get() functions...

Ugh, you are right. We cannot get rid of the getters. For now, forget about my
comments about getting getters...

I think ToString() comment is still valid.

[Simon Que, 2011-10-27 20:47:34]

PTAL

[satorux1, 2011-10-27 21:09:21]

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/mock_power_library.h (right):

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mock_power_library.h:32: virtual void
UpdatePowerStatus(const PowerStatus&) {}
MOCK_METHOD1(UpdatePowerStatus, void(const PowerStatus&));

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

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:122: FOR_EACH_OBSERVER(Observer,
observers_, PowerChanged(status));
The method is essentially redirecting UpdatePowerStatus(status) to
PowerChanged(status). 

I think we can get rid of this redirection by changing the clients of
PowerLibrary to directly use PowerManagerClient, and that'd be simpler.

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:14: #include
"third_party/cros/chromeos_power.h"
We don't need this, right?

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:16: // TODO(sque): Remove
chromeos_power.h, crosbug.com/16558
ditto.

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:150:
base::Bind(&PowerManagerClientImpl::OnPowerSupplyPoll,
OnPowerSupplyPoll -> OnGetAllPropertiesMethod to match the method name?

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:154: void
OnPowerSupplyPoll(dbus::Response* response) {
Please add a function comment like

// Called when xxxx method is complete.

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:157: << ".";
Let's omit the period to save one line.

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:177: :
BATTERY_STATE_UNKNOWN;
remove a space before : ?

[Simon Que, 2011-10-27 22:46:19]

PTAL ... more yak shaving!

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/mock_power_library.h (right):

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mock_power_library.h:32: virtual void
UpdatePowerStatus(const PowerStatus&) {}
On 2011/10/27 21:09:21, satorux1 wrote:
> MOCK_METHOD1(UpdatePowerStatus, void(const PowerStatus&));

Done.

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

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:122: FOR_EACH_OBSERVER(Observer,
observers_, PowerChanged(status));
On 2011/10/27 21:09:21, satorux1 wrote:
> The method is essentially redirecting UpdatePowerStatus(status) to
> PowerChanged(status). 
> 
> I think we can get rid of this redirection by changing the clients of
> PowerLibrary to directly use PowerManagerClient, and that'd be simpler.

Done.

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:14: #include
"third_party/cros/chromeos_power.h"
On 2011/10/27 21:09:21, satorux1 wrote:
> We don't need this, right?

Done.

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:16: // TODO(sque): Remove
chromeos_power.h, crosbug.com/16558
On 2011/10/27 21:09:21, satorux1 wrote:
> ditto.

Done.

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:150:
base::Bind(&PowerManagerClientImpl::OnPowerSupplyPoll,
On 2011/10/27 21:09:21, satorux1 wrote:
> OnPowerSupplyPoll -> OnGetAllPropertiesMethod to match the method name?

Done.

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:154: void
OnPowerSupplyPoll(dbus::Response* response) {
On 2011/10/27 21:09:21, satorux1 wrote:
> Please add a function comment like
> 
> // Called when xxxx method is complete.

Done.

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:157: << ".";
On 2011/10/27 21:09:21, satorux1 wrote:
> Let's omit the period to save one line.

Done.

http://codereview.chromium.org/8271024/diff/10001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:177: :
BATTERY_STATE_UNKNOWN;
On 2011/10/27 21:09:21, satorux1 wrote:
> remove a space before : ?

Done.

[satorux1, 2011-10-28 17:36:23]

http://codereview.chromium.org/8271024/diff/10007/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/8271024/diff/10007/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:16: #include
"chrome/browser/chromeos/cros/power_library.h"
Do we need this?

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

http://codereview.chromium.org/8271024/diff/10007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:15: #include
"chrome/browser/chromeos/dbus/dbus_thread_manager.h"
We don't need this, right?

http://codereview.chromium.org/8271024/diff/10007/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/8271024/diff/10007/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:183: // Called when
GetAllPropertiesMethod call is complete.
Move the function comment to the out of the function:

 // Called when GetAllPropertiesMethod call is complete.
 void OnGetAllPropertiesMethod(dbus::Response* response) {

http://codereview.chromium.org/8271024/diff/10007/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:192: double
battery_energy_rate;
Let's initialize the doubles with 0.0; These seem to be unused so renaming to
unsed_battery_voltage, would also be better.

http://codereview.chromium.org/8271024/diff/10007/chrome/browser/chromeos/low...
File chrome/browser/chromeos/low_battery_observer.h (right):

http://codereview.chromium.org/8271024/diff/10007/chrome/browser/chromeos/low...
chrome/browser/chromeos/low_battery_observer.h:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
2011

[Simon Que, 2011-10-29 02:20:11]

PTAl

http://codereview.chromium.org/8271024/diff/10007/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/8271024/diff/10007/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:16: #include
"chrome/browser/chromeos/cros/power_library.h"
On 2011/10/28 17:36:23, satorux1 wrote:
> Do we need this?

Done.

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

http://codereview.chromium.org/8271024/diff/10007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:15: #include
"chrome/browser/chromeos/dbus/dbus_thread_manager.h"
On 2011/10/28 17:36:23, satorux1 wrote:
> We don't need this, right?

Done.

http://codereview.chromium.org/8271024/diff/10007/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/8271024/diff/10007/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:183: // Called when
GetAllPropertiesMethod call is complete.
On 2011/10/28 17:36:23, satorux1 wrote:
> Move the function comment to the out of the function:
> 
>  // Called when GetAllPropertiesMethod call is complete.
>  void OnGetAllPropertiesMethod(dbus::Response* response) {

Done.

http://codereview.chromium.org/8271024/diff/10007/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:192: double
battery_energy_rate;
On 2011/10/28 17:36:23, satorux1 wrote:
> Let's initialize the doubles with 0.0; These seem to be unused so renaming to
> unsed_battery_voltage, would also be better.

Done.

http://codereview.chromium.org/8271024/diff/10007/chrome/browser/chromeos/low...
File chrome/browser/chromeos/low_battery_observer.h (right):

http://codereview.chromium.org/8271024/diff/10007/chrome/browser/chromeos/low...
chrome/browser/chromeos/low_battery_observer.h:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
On 2011/10/28 17:36:23, satorux1 wrote:
> 2011

Done.

[satorux1, 2011-10-29 02:28:48]

http://codereview.chromium.org/8271024/diff/12015/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/8271024/diff/12015/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:144: // implementation.
Please add some comment that this needs to be done after
DBusThreadManager::Initialize()

http://codereview.chromium.org/8271024/diff/12015/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

http://codereview.chromium.org/8271024/diff/12015/content/browser/browser_mai...
content/browser/browser_main_loop.cc:249: #if !defined(OS_CHROMEOS)
This looks wrong. network_change_notifier_ will be empty for Chrome OS...

Let me think what would be the right way to fix this.

[Simon Que, 2011-11-02 00:13:43]

http://codereview.chromium.org/8271024/diff/12015/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/8271024/diff/12015/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:144: // implementation.
On 2011/10/29 02:28:48, satorux1 wrote:
> Please add some comment that this needs to be done after
> DBusThreadManager::Initialize()

Done.

[satorux1, 2011-11-02 05:07:48]

http://codereview.chromium.org/8271024/diff/18002/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

http://codereview.chromium.org/8271024/diff/18002/content/browser/browser_mai...
content/browser/browser_main_loop.cc:255:
network_change_notifier_.reset(net::NetworkChangeNotifier::Create());
I thought we wanted to move this to RunMainMessageLoopParts()? Besides, we
should add comment why we do this after PostMainMessageLoopStart().

http://codereview.chromium.org/8271024/diff/18002/content/browser/browser_mai...
content/browser/browser_main_loop.cc:262: 
somewhere like here?

[satorux1, 2011-11-02 05:09:04]

Tried to run try bots, but git cl patch failed. Please rebase and re-upload.


% git cl patch 8271024 
Loaded authentication cookies from /home/satorux/.codereview_upload_cookies
error: patch failed: chrome/browser/chromeos/chrome_browser_main_chromeos.cc:120
error: chrome/browser/chromeos/chrome_browser_main_chromeos.cc: patch does not
apply
Failed to apply the patch

[Simon Que, 2011-11-02 21:56:26]

Rebased, PTAL

[Simon Que, 2011-11-03 23:23:22]

PTAL

[satorux1, 2011-11-03 23:38:47]

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:147: new
chromeos::CrosNetworkChangeNotifierFactory());
Please move this back to the original place. We no longer need to move.

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

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:62:
chromeos::EnableScreenLock(enable);
Why this function is changed? The change does not look correct. Please revert
the change here.

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:73: virtual void
RequestStatusUpdate() OVERRIDE {
Can you remove this? You now have the same name function in
power_manager_client.cc.

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:76: // when we migrate the DBus
code from libcros to here.
Please move the comments to power_manager_client.cc

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:109: 
Let's not add a blank line here.

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:7: #include <inttypes.h>
Why is this needed?  base/basictypes.h instead?

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:117: virtual void
RequestStatusUpdate() {}
See my commments in power_library.cc

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/power_manager_client.h (right):

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.h:40: battery_percentage(0) {}
linux_chromeos_clang build tends to complain about inlined constructors. Maybe
safer to move this to .cc file.

[satorux1, 2011-11-04 22:24:56]

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/power_manager_client.h (right):

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.h:73: virtual void
RequestStatusUpdate() = 0;
Please add this to mock_power_manager_client.h too.

[Simon Que, 2011-11-04 22:35:31]

PTAL

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:147: new
chromeos::CrosNetworkChangeNotifierFactory());
On 2011/11/03 23:38:47, satorux1 wrote:
> Please move this back to the original place. We no longer need to move.

Done.

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

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:62:
chromeos::EnableScreenLock(enable);
On 2011/11/03 23:38:47, satorux1 wrote:
> Why this function is changed? The change does not look correct. Please revert
> the change here.

Done.

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:73: virtual void
RequestStatusUpdate() OVERRIDE {
On 2011/11/03 23:38:47, satorux1 wrote:
> Can you remove this? You now have the same name function in
> power_manager_client.cc.
> 

Done.

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:76: // when we migrate the DBus
code from libcros to here.
On 2011/11/03 23:38:47, satorux1 wrote:
> Please move the comments to power_manager_client.cc

Done.

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.cc:109: 
On 2011/11/03 23:38:47, satorux1 wrote:
> Let's not add a blank line here.

Done.

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:7: #include <inttypes.h>
On 2011/11/03 23:38:47, satorux1 wrote:
> Why is this needed?  base/basictypes.h instead?

It's used for "PRId64" in ToString(), since we use int64.

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:117: virtual void
RequestStatusUpdate() {}
On 2011/11/03 23:38:47, satorux1 wrote:
> See my commments in power_library.cc

Done.

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/power_manager_client.h (right):

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.h:40: battery_percentage(0) {}
On 2011/11/03 23:38:47, satorux1 wrote:
> linux_chromeos_clang build tends to complain about inlined constructors. Maybe
> safer to move this to .cc file.

Done.

http://codereview.chromium.org/8271024/diff/25001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.h:73: virtual void
RequestStatusUpdate() = 0;
On 2011/11/04 22:24:57, satorux1 wrote:
> Please add this to mock_power_manager_client.h too.

Done.

[satorux1, 2011-11-04 22:52:30]

almost there!

http://codereview.chromium.org/8271024/diff/35002/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/8271024/diff/35002/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:126: // implementation. 
This should be done after DBusThreadManager init.
"  This should be done after DBusThreadManager init." is no longer relevant.
Please remove.

http://codereview.chromium.org/8271024/diff/35002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/power_library.h (left):

http://codereview.chromium.org/8271024/diff/35002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.h:76: virtual void
RequestStatusUpdate() = 0;
You should remove this from mock_power_library.h in the same directory.

[Simon Que, 2011-11-04 22:57:59]

PTAL

http://codereview.chromium.org/8271024/diff/35002/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/8271024/diff/35002/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:126: // implementation. 
This should be done after DBusThreadManager init.
On 2011/11/04 22:52:30, satorux1 wrote:
> "  This should be done after DBusThreadManager init." is no longer relevant.
> Please remove.

Done.

http://codereview.chromium.org/8271024/diff/35002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/power_library.h (left):

http://codereview.chromium.org/8271024/diff/35002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.h:76: virtual void
RequestStatusUpdate() = 0;
On 2011/11/04 22:52:30, satorux1 wrote:
> You should remove this from mock_power_library.h in the same directory.

Done.

[satorux1, 2011-11-04 23:04:54]

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

http://codereview.chromium.org/8271024/diff/39001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.h:12: #include
"chrome/browser/chromeos/dbus/power_manager_client.h"
I think you don't need this?

http://codereview.chromium.org/8271024/diff/39001/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/8271024/diff/39001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:22:
PowerSupplyStatus::PowerSupplyStatus() : line_power_on(false),
The formatting looks unusual. It's usually like:

PowerSupplyStatus::PowerSupplyStatus()
    : line_power_on(false),
      ...
      battery_percentage(0) {
}

[Simon Que, 2011-11-04 23:13:37]

PTAL

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

http://codereview.chromium.org/8271024/diff/39001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/power_library.h:12: #include
"chrome/browser/chromeos/dbus/power_manager_client.h"
On 2011/11/04 23:04:54, satorux1 wrote:
> I think you don't need this?

Done.

http://codereview.chromium.org/8271024/diff/39001/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/8271024/diff/39001/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:22:
PowerSupplyStatus::PowerSupplyStatus() : line_power_on(false),
On 2011/11/04 23:04:54, satorux1 wrote:
> The formatting looks unusual. It's usually like:
> 
> PowerSupplyStatus::PowerSupplyStatus()
>     : line_power_on(false),
>       ...
>       battery_percentage(0) {
> }

Done.

[satorux1, 2011-11-04 23:19:22]

LGTM

[Simon Que, 2011-11-07 20:14:17]

PTAL

[Simon Que, 2011-11-07 22:22:01]

PTAL

[satorux1, 2011-11-10 06:18:51]

LGTM. Didn't show up here, but linux_chromeos try bot passed with the latest
patch

[commit-bot: I haz the power, 2011-11-10 21:40:11]

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

[commit-bot: I haz the power, 2011-11-10 21:40:21]

Presubmit check for 8271024-55002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/chromeos/status/status_area_view_chromeos.cc,chrome/browser/chromeos/status/power_menu_button.cc,chrome/browser/chromeos/status/power_menu_button.h,chrome/browser/chromeos/status/status_area_view_chromeos.h

Presubmit checks took 2.8s to calculate.

[stevenjb, 2011-11-10 21:46:24]

Status area LGTM

[commit-bot: I haz the power, 2011-11-10 21:46:56]

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

[commit-bot: I haz the power, 2011-11-10 23:02:00]

Change committed as 109529

[Nico, 2011-11-20 22:38:21]

http://codereview.chromium.org/8271024/diff/55002/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/8271024/diff/55002/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:50:
battery_seconds_to_full);
This will append "line_power_on" to result every time this function runs, and
since result is static, the string will get longer and longer. Is that intended?
If not, the best fix is probably to make result not static and let the function
return by copy instead of by const reference.

[satorux1, 2011-11-21 17:28:05]

http://codereview.chromium.org/8271024/diff/55002/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/8271024/diff/55002/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/power_manager_client.cc:50:
battery_seconds_to_full);
On 2011/11/20 22:38:21, Nico wrote:
> This will append "line_power_on" to result every time this function runs, and
> since result is static, the string will get longer and longer. Is that
intended?
> If not, the best fix is probably to make result not static and let the
function
> return by copy instead of by const reference.

Good catch! I wasn't able to catch this. Simon, please create a patch to remove
'static', and change the return value type to std::string as Nico suggested.