Fix off-by-one bug for the negative transition in XIdle::AddIdleTimeout, which

Fix off-by-one bug for the negative transition in XIdle::AddIdleTimeout, which
causes mouse and keyboard movements by the user to sometimes be missed. Add
unit test to verify correct behavior.
BUG=none
TEST=included

[Daniel Erat, 2010-03-16 04:29:25]

I saved more comments but Rietveld lost them.

http://codereview.chromium.org/1035001/diff/16001/17003
File src/platform/power_manager/xidle.cc (right):

http://codereview.chromium.org/1035001/diff/16001/17003#newcode33
src/platform/power_manager/xidle.cc:33: if (display_ &&
i think that you should just crash loudly if a NULL display gets passed in; the
caller should be checking this first

http://codereview.chromium.org/1035001/diff/16001/17005
File src/platform/power_manager/xidle_unittest.cc (right):

http://codereview.chromium.org/1035001/diff/16001/17005#newcode8
src/platform/power_manager/xidle_unittest.cc:8: #include <base/command_line.h>
standardize <> vs "" -- i believe we use <> for headers under /usr and "" for
headers within the repository.  see e.g.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...

http://codereview.chromium.org/1035001/diff/16001/17005#newcode23
src/platform/power_manager/xidle_unittest.cc:23: Display* display =
XOpenDisplay(NULL);
the style guide says that static or global variables of class type are
forbidden.  can you set this up and tear it down within the XIdleTest class
instead?

[davidjames, 2010-03-16 17:43:47]

Dan, thank you for your patience! I really appreciate your help on this.

I've addressed your comments in the latest version of my patch. PTAL.

[Daniel Erat, 2010-03-16 21:40:00]

LGTM

http://codereview.chromium.org/1035001/diff/24001/25001
File src/platform/power_manager/SConstruct (right):

http://codereview.chromium.org/1035001/diff/24001/25001#newcode42
src/platform/power_manager/SConstruct:42: test_env.Append(LIBS=['gtest','Xtst'])
nit: add a space after the comma

http://codereview.chromium.org/1035001/diff/24001/25002
File src/platform/power_manager/xidle-example.cc (right):

http://codereview.chromium.org/1035001/diff/24001/25002#newcode8
src/platform/power_manager/xidle-example.cc:8: #include "base/logging.h"
alphabetize these?

http://codereview.chromium.org/1035001/diff/24001/25002#newcode12
src/platform/power_manager/xidle-example.cc:12: Display* display =
XOpenDisplay(NULL);
it'd be good to CHECK(display) here; that's what X apps usually do (to catch the
common case where someone tries to run it when no X server is present or when
$DISPLAY is set incorrectly)

http://codereview.chromium.org/1035001/diff/24001/25003
File src/platform/power_manager/xidle.cc (right):

http://codereview.chromium.org/1035001/diff/24001/25003#newcode59
src/platform/power_manager/xidle.cc:59: XSyncAlarm alarm =
CreateIdleAlarm(min_timeout_ - 1,
it'd be good to add a comment here describing the race condition that's being
worked around

http://codereview.chromium.org/1035001/diff/24001/25005
File src/platform/power_manager/xidle_unittest.cc (right):

http://codereview.chromium.org/1035001/diff/24001/25005#newcode31
src/platform/power_manager/xidle_unittest.cc:31: XTestFakeMotionEvent(display,
0, 0, 0, 0);
i think that this will warp the user's pointer to (0, 0) if they're running this
with their real X server, right?  that seems like it may be undesirable.  does
XTestFakeRelativeMotionEvent(display, 0, 0, 0, 0) also trigger the alarm?

[davidjames, 2010-03-16 22:27:09]

http://codereview.chromium.org/1035001/diff/24001/25005
File src/platform/power_manager/xidle_unittest.cc (right):

http://codereview.chromium.org/1035001/diff/24001/25005#newcode31
src/platform/power_manager/xidle_unittest.cc:31: XTestFakeMotionEvent(display,
0, 0, 0, 0);
On 2010/03/16 21:40:01, Daniel Erat wrote:
> i think that this will warp the user's pointer to (0, 0) if they're running
this
> with their real X server, right?  that seems like it may be undesirable.  does
> XTestFakeRelativeMotionEvent(display, 0, 0, 0, 0) also trigger the alarm?

Aha, good catch. I didn't notice the warping because I was running with a fake X
server. The fake relative motion event is, in fact, good enough to trigger the
alarm. I committed the change with all the fixes you suggested (including the
relative motion event change).