Reland old fix that was reverted without my knowledge.

Reland old fix that was reverted incorrectly.

Fix regression where high resolution timers could be activated even under
battery power. Add unit test to protect chromium from developers like me
in the future.

The fix is a one-liner in hi_res_timer_manager_win.cc. The rest of the code
change is the mechanics to enable the unit test.

BUG=59528
TEST=HiResTimerManagerTest.ToggleOnOff
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85413

[Mike Belshe, 2011-04-29 15:26:37]

Jim - 

You code reviewed this long ago:
http://codereview.chromium.org/3848002/show

But it got reverted multiple times due to unrelated flakey tests, and that
caused it to get dropped entirely.

I'm relanding, finally. The code is all the same, except that I had to re-merge
it because code has shifted since Oct when this happened.

A sad story.

[jar (doing other things), 2011-05-01 17:42:34]

Question and a nit below.

http://codereview.chromium.org/6904117/diff/3011/base/time_win.cc
File base/time_win.cc (right):

http://codereview.chromium.org/6904117/diff/3011/base/time_win.cc#newcode176
base/time_win.cc:176: high_resolution_timer_activated_++;
Should you be concerned with atomic increment and decrement?  Is this guarnateed
to only appear on one thread?

http://codereview.chromium.org/6904117/diff/3011/content/common/hi_res_timer_...
File content/common/hi_res_timer_manager_unittest.cc (right):

http://codereview.chromium.org/6904117/diff/3011/content/common/hi_res_timer_...
content/common/hi_res_timer_manager_unittest.cc:24:
manager.OnPowerStateChange(/* on_battery */ false);
Rather than using a boolean, it would be much more readable to use a pair of
distinct enumerators.  (a new pet peeve I've added to my code reviews when the
function or method name does not make the meaning of the argument clear).

[Mike Belshe, 2011-05-01 21:58:00]

http://codereview.chromium.org/6904117/diff/3011/base/time_win.cc
File base/time_win.cc (right):

http://codereview.chromium.org/6904117/diff/3011/base/time_win.cc#newcode176
base/time_win.cc:176: high_resolution_timer_activated_++;
On 2011/05/01 17:42:34, jar wrote:
> Should you be concerned with atomic increment and decrement?  Is this
guarnateed
> to only appear on one thread?

Good point, I'll add atomics.

http://codereview.chromium.org/6904117/diff/3011/content/common/hi_res_timer_...
File content/common/hi_res_timer_manager_unittest.cc (right):

http://codereview.chromium.org/6904117/diff/3011/content/common/hi_res_timer_...
content/common/hi_res_timer_manager_unittest.cc:24:
manager.OnPowerStateChange(/* on_battery */ false);
On 2011/05/01 17:42:34, jar wrote:
> Rather than using a boolean, it would be much more readable to use a pair of
> distinct enumerators.  (a new pet peeve I've added to my code reviews when the
> function or method name does not make the meaning of the argument clear).

Well, if I'm going to change the API and all callers of it, would it be better
to change it to "OnBattery(bool)"?

[Mike Belshe, 2011-05-04 07:23:13]

http://codereview.chromium.org/6904117/diff/3011/base/time_win.cc
File base/time_win.cc (right):

http://codereview.chromium.org/6904117/diff/3011/base/time_win.cc#newcode176
base/time_win.cc:176: high_resolution_timer_activated_++;
On 2011/05/01 21:58:00, Mike Belshe wrote:
> On 2011/05/01 17:42:34, jar wrote:
> > Should you be concerned with atomic increment and decrement?  Is this
> guarnateed
> > to only appear on one thread?
> 
> Good point, I'll add atomics.


Actually, Jim - I looked again.  The comments in lines 186-190 explain this, and
the comments in the header file do as well.  This is only used for testing (so
we can verify the high res timer is activated or not), and the comments clearly
say that this can only be used in single-threaded tests.

[jar (doing other things), 2011-05-04 16:08:26]

LGTM with with the two items at your discretion.

http://codereview.chromium.org/6904117/diff/3011/base/time_win.cc
File base/time_win.cc (right):

http://codereview.chromium.org/6904117/diff/3011/base/time_win.cc#newcode176
base/time_win.cc:176: high_resolution_timer_activated_++;
Ah... I see... sorry for my confusion.

Maybe it would be good to add comments for lines 176 and 189:

  // For tests ONLY.  Not used in production.

I would expect that you'll have to have a suppression in valgrind for this,
since the value will be changed in a race, even though never relied upon. (maybe
you already have a suppression).  Having a local comment would make it clear
that a suppression should be added.

On 2011/05/04 07:23:13, Mike Belshe wrote:
> On 2011/05/01 21:58:00, Mike Belshe wrote:
> > On 2011/05/01 17:42:34, jar wrote:
> > > Should you be concerned with atomic increment and decrement?  Is this
> > guarnateed
> > > to only appear on one thread?
> > 
> > Good point, I'll add atomics.
> 
> 
> Actually, Jim - I looked again.  The comments in lines 186-190 explain this,
and
> the comments in the header file do as well.  This is only used for testing (so
> we can verify the high res timer is activated or not), and the comments
clearly
> say that this can only be used in single-threaded tests.
>

http://codereview.chromium.org/6904117/diff/3011/content/common/hi_res_timer_...
File content/common/hi_res_timer_manager_unittest.cc (right):

http://codereview.chromium.org/6904117/diff/3011/content/common/hi_res_timer_...
content/common/hi_res_timer_manager_unittest.cc:24:
manager.OnPowerStateChange(/* on_battery */ false);
Yeah... a name change like that would be an equally good (or better)
improvement.  The prefix word "On" has come to mean "When an event ... has
happened" so I'd probably rather see:

BatteryOn(bool state)

or

BatteryIsOn(bool state)

or

BatteryPowered(bool is_battery_powered)

or something of that sort.

On 2011/05/01 21:58:00, Mike Belshe wrote:
> On 2011/05/01 17:42:34, jar wrote:
> > Rather than using a boolean, it would be much more readable to use a pair of
> > distinct enumerators.  (a new pet peeve I've added to my code reviews when
the
> > function or method name does not make the meaning of the argument clear).
> 
> Well, if I'm going to change the API and all callers of it, would it be better
> to change it to "OnBattery(bool)"?
>