Increased battery time to empty sent by power manager stub impl

Increased battery time to empty sent by power manager client stub implementation.

TEST=none
BUG=none

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

[Simon Que, 2011-11-14 21:40:15]

On 2011/11/14 21:39:32, tbarzic wrote:

LGTM

[satorux1, 2011-11-14 22:03:04]

http://codereview.chromium.org/8555018/diff/2001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/8555018/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/power_manager_client.cc:317: std::max(16,
battery_percentage_ * 180 / 100);
Why did we end up showing the low battery balloon in browser_tests from the
beginning?

Looking at the code, the battery percentage started from 80%, and decreases by 1
point every time Update() is called.

Update() is called by a repeating timer every 100 ms from the first time
RequestStatusUpdate() is called. Hence it only needed 8 seconds to drain the
battery.

I'd say 100ms is too aggressive. We could make it something like 5 seconds for
now.

Besides, the magic number 180 looks cryptic. This is the number of  seconds to
fully charge the battery, right? 180 looks unrealistically short. I think we
should define a constant with a more realistic number, like

kNumSecondsToFullyChargeBattery = 60 * 120;  // 120 mins.

[satorux1, 2011-11-14 22:10:41]

http://codereview.chromium.org/8555018/diff/2001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/8555018/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/power_manager_client.cc:317: std::max(16,
battery_percentage_ * 180 / 100);
On 2011/11/14 22:03:05, satorux1 wrote:
> Why did we end up showing the low battery balloon in browser_tests from the
> beginning?
> 
> Looking at the code, the battery percentage started from 80%, and decreases by
1
> point every time Update() is called.
> 
> Update() is called by a repeating timer every 100 ms from the first time
> RequestStatusUpdate() is called. Hence it only needed 8 seconds to drain the
> battery.
> 
> I'd say 100ms is too aggressive. We could make it something like 5 seconds for
> now.
> 
> Besides, the magic number 180 looks cryptic. This is the number of  seconds to
> fully charge the battery, right? 180 looks unrealistically short. I think we
> should define a constant with a more realistic number, like
> 
> kNumSecondsToFullyChargeBattery = 60 * 120;  // 120 mins.

FWIW, it used to be 3 hours, but changed to 180 seconds in this patch:

http://codereview.chromium.org/8347016/diff/27003/chrome/browser/chromeos/cro...

Which is partly my fault as I wasn't able to catch the undesirable change.

[satorux1, 2011-11-14 22:59:49]

http://codereview.chromium.org/8555018/diff/6001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/8555018/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/power_manager_client.cc:251:
kSecondsToEmptyFullBattery(3 * 60 * 60) /* 3 hours */ {
It's awkward to make it a member variable. see below.

http://codereview.chromium.org/8555018/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/power_manager_client.cc:311: PowerSupplyStatus
status;
You can define the constant here:

const int kSecondsToEmptyFullBattery = 3 * 60 * 60;

Alternatively, you can define this in anonymous namespace.

[tbarzic, 2011-11-14 23:05:39]

http://codereview.chromium.org/8555018/diff/6001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/8555018/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/power_manager_client.cc:251:
kSecondsToEmptyFullBattery(3 * 60 * 60) /* 3 hours */ {
On 2011/11/14 22:59:50, satorux1 wrote:
> It's awkward to make it a member variable. see below.

Done.

http://codereview.chromium.org/8555018/diff/6001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/power_manager_client.cc:311: PowerSupplyStatus
status;
On 2011/11/14 22:59:50, satorux1 wrote:
> You can define the constant here:
> 
> const int kSecondsToEmptyFullBattery = 3 * 60 * 60;
> 
> Alternatively, you can define this in anonymous namespace.

Done.

[satorux1, 2011-11-14 23:46:57]

LGTM with nits.

http://codereview.chromium.org/8555018/diff/7002/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/8555018/diff/7002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/power_manager_client.cc:304: pause_count_ = 2;
Not your fault, but this logic looks rather awkward. You set 2 here, but we
decrement this counter only once at the beginning of the function, right? Adding
a comment like below may make it clear.

  // Pause twice (i.e. skip updating the menu), including the current call to
this function.
  pause_count_ = 2;

http://codereview.chromium.org/8555018/diff/7002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/power_manager_client.cc:310: const int
kSecondsToEmptyFullBattery(3 * 60 * 60); // 3 hours.
two spaces before // per our style guide.