Fix regression where high resolution timers could be activated even under...

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

[jar (doing other things), 2010-10-18 06:34:16]

One real sematic/api question, and two nits below.

http://codereview.chromium.org/3848002/diff/16001/17002
File app/hi_res_timer_manager.h (right):

http://codereview.chromium.org/3848002/diff/16001/17002#newcode22
app/hi_res_timer_manager.h:22: bool using_hi_res_clock() { return
hi_res_clock_used_; }
nit: Declare this as a const method.

http://codereview.chromium.org/3848002/diff/16001/17007
File base/time_win.cc (right):

http://codereview.chromium.org/3848002/diff/16001/17007#newcode167
base/time_win.cc:167: if (!high_resolution_timer_enabled_)
I'm concerned about the API, and the semantics of this function, given line 167.

The problem is that when high_resolution_timer_enabled_ is false, it is
impossible to do anything via this function call.  This means that the counter
can be pushed up.... then this mode diabled... and THEN an attempt to back out
of an increase in resolution will be ignored (i.e., there is no way to cause the
downcounting of high_resolution_timer_activated_).

It is more plausible that this function should always do downcounting (and
upcounting), and that the boolean should override the state.  

What is the expected use case for this function?  Is the caller expected to
realize that a return of false means nothing happened... and hence it should
queue up such requests for later??

Am I misunderstanding how this is used completely?

http://codereview.chromium.org/3848002/diff/16001/17007#newcode187
base/time_win.cc:187: high_resolution_timer_activated_ > 0;
nit: Despite the fact that this read nicely IMO, I think you're supposed to wrap
indentations 4 characters (unless you have a paren to align into).

[Mike Belshe, 2010-10-18 21:29:12]

http://codereview.chromium.org/3848002/diff/16001/17002
File app/hi_res_timer_manager.h (right):

http://codereview.chromium.org/3848002/diff/16001/17002#newcode22
app/hi_res_timer_manager.h:22: bool using_hi_res_clock() { return
hi_res_clock_used_; }
On 2010/10/18 06:34:17, jar wrote:
> nit: Declare this as a const method.

Done.

http://codereview.chromium.org/3848002/diff/16001/17007
File base/time_win.cc (right):

http://codereview.chromium.org/3848002/diff/16001/17007#newcode167
base/time_win.cc:167: if (!high_resolution_timer_enabled_)
On 2010/10/18 06:34:17, jar wrote:
> I'm concerned about the API, and the semantics of this function, given line
167.
> 
> The problem is that when high_resolution_timer_enabled_ is false, it is
> impossible to do anything via this function call.  This means that the counter
> can be pushed up.... then this mode diabled... and THEN an attempt to back out
> of an increase in resolution will be ignored (i.e., there is no way to cause
the
> downcounting of high_resolution_timer_activated_).

Yes, this did occur to me as well.  I had glossed over it because I really
shouldn't be tracking this second counter at all.  The purpose is solely for
tracking within the unit tests.  Nonetheless, as I think about it more, I think
I should fix it.

> 
> It is more plausible that this function should always do downcounting (and
> upcounting), and that the boolean should override the state.  
> 
> What is the expected use case for this function?  Is the caller expected to
> realize that a return of false means nothing happened... and hence it should
> queue up such requests for later??
> 
> Am I misunderstanding how this is used completely?

http://codereview.chromium.org/3848002/diff/16001/17007#newcode187
base/time_win.cc:187: high_resolution_timer_activated_ > 0;
On 2010/10/18 06:34:17, jar wrote:
> nit: Despite the fact that this read nicely IMO, I think you're supposed to
wrap
> indentations 4 characters (unless you have a paren to align into).

Done.

[jar (doing other things), 2010-10-18 23:35:25]

http://codereview.chromium.org/3848002/diff/17008/2004
File app/hi_res_timer_manager_unittest.cc (right):

http://codereview.chromium.org/3848002/diff/17008/2004#newcode37
app/hi_res_timer_manager_unittest.cc:37: 
You should have another test where you try to toggle the state here at line 37,
instead of at line 44.  The question is, will the state get toggled properly?

http://codereview.chromium.org/3848002/diff/17008/2008
File base/time_win.cc (right):

http://codereview.chromium.org/3848002/diff/17008/2008#newcode175
base/time_win.cc:175: result = timeBeginPeriod(kMinTimerIntervalMs);
My fear now is that you're not calling the begin (line 175) as often as you may
call the End (line 179).

Will the OS handle all this?.... and what will its "handling" do??

[Mike Belshe, 2010-10-19 22:48:41]

Ok - lots of iterations through the weeds and back to sanity.

The message loop wasn't really perfect before - it wasn't tracking whether it
had activated the timer or not (due to timers being off), and as such, it could
have called timeEndPeriod without a corresponding timeBeginPeriod (as you
pointed out.  Fixed that.

The flag in the Time class is not thread safe - but that is okay - because it's
just for verifying this functionality, which couldn't be tested without a little
help.

Anyway - hopefully good to go now.

http://codereview.chromium.org/3848002/diff/17008/2004
File app/hi_res_timer_manager_unittest.cc (right):

http://codereview.chromium.org/3848002/diff/17008/2004#newcode37
app/hi_res_timer_manager_unittest.cc:37: 
On 2010/10/18 23:35:25, jar wrote:
> You should have another test where you try to toggle the state here at line
37,
> instead of at line 44.  The question is, will the state get toggled properly?

this gets tested as we come back around to the top of the loop - notice that
lines 24/27 will test these conditions right after having executed  line 44.

[jar (doing other things), 2010-10-20 00:37:18]

LGTM (with nits below)

http://codereview.chromium.org/3848002/diff/54001/55003
File app/hi_res_timer_manager_unittest.cc (right):

http://codereview.chromium.org/3848002/diff/54001/55003#newcode28
app/hi_res_timer_manager_unittest.cc:28:
EXPECT_TRUE(manager.using_hi_res_clock());
nit: Suggest rename:

hi_res_clock_allowable

http://codereview.chromium.org/3848002/diff/54001/55005
File base/message_loop.cc (right):

http://codereview.chromium.org/3848002/diff/54001/55005#newcode363
base/message_loop.cc:363: if (base::TimeTicks::Now() >
high_resolution_timer_expiration_) {
This will work.... but for all your effort to avoid taking a lock, Now() will
take a lock :-/. 

It would be better to put this down in the timer event handling code, but that
is a more complex change :-(.  I'm working on such a change (for other
appearances of Now() in the message loop), and that should probably be enhanced
to include this "timer event" (or perhaps we should post a delayed event to
handle this, and let it go down the standard track).

http://codereview.chromium.org/3848002/diff/54001/55006
File base/time.h (right):

http://codereview.chromium.org/3848002/diff/54001/55006#newcode415
base/time.h:415: // in a thread safe way.
Please add to comment:
...and used ONLY by tests which are running single threaded.