Iplement the idle timer for Linux using libxss.

Implement the idle timer for Linux using libxss.

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

[dsh, 2008-10-27 22:24:29]

I'm still waiting for the trybots to get the libxss stuff installed so I can
test officially, but this should be close enough for a review.

Thanks!

[Evan Martin, 2008-10-27 22:44:05]

http://codereview.chromium.org/8806/diff/1/2
File base/idle_timer.cc (right):

http://codereview.chromium.org/8806/diff/1/2#newcode50
Line 50: #elif defined(OS_LINUX)
Would it make sense to just split this into multiple files?

http://codereview.chromium.org/8806/diff/1/2#newcode51
Line 51: struct XssInfo {
If it's possible, I'd prefer to avoid abbreviations.

http://codereview.chromium.org/8806/diff/1/2#newcode53
Line 53: ThreadLocalPointer<XScreenSaverInfo*> xss_info;
Two things called xss_info makes this code kinda confusing.  Maybe the struct
could be called IdleInfo or something?

http://codereview.chromium.org/8806/diff/1/2#newcode74
Line 74: bool OSIdleTimeSource(int32 *milliseconds_interval_since_last_event) {
star on the left

[dsh, 2008-11-04 23:43:32]

Cleaned up per previous comments.

[Evan Martin, 2008-11-06 00:35:43]

I think this change in isolation looks good, but having thought about it a bit
I'm a bit skeptical we want to even do this.  As far as I can tell the IdleTimer
in Chrome is only used for a pretty marginal case which may not even be
implementable on Linux.

So I don't know what to do with this review, then.  Adding that extra library is
one more dependency, increases our link time, and increases our start time in
the dynamic linker.

Maybe you could commit it with it all #ifdef'd out and we could decide later? 
Sorry I didn't say anything sooner... :\

http://codereview.chromium.org/8806/diff/5/6
File base/base_unittests.scons (right):

http://codereview.chromium.org/8806/diff/5/6#newcode66
Line 66: 'Xss'
Could you put a comment by this describing what it is?

[dsh, 2008-11-06 02:05:01]

OK, I'm not 100% sure how you wanted me to comment this out style-wise; how's
this?

[Dean McNamee, 2008-11-06 08:57:05]

http://codereview.chromium.org/8806/diff/11/13
File base/idle_timer.cc (right):

http://codereview.chromium.org/8806/diff/11/13#newcode62
Line 62: static void New(void* instance) {
This code is not technically correct.  You never construct (or destroy)
IdleState.  Look at other examples of how to use the LazyInstance traits.  I
realize it's ok here, because LazyInstance preallocates the space, and it will
be initialized to 0, however, it is not designed to be used like that.  You have
to use placement new...

However, you control the struct completely.  So it is best not to use traits at
all (I encourage avoiding traits for a lot of reasons).  Move the New() to be
the IdleState() constructor, and Delete() to be it's destructor.

[Evan Martin, 2008-11-06 20:28:19]

Minor comment; the rest LGTM after you make Dean happy. :)

http://codereview.chromium.org/8806/diff/11/13
File base/idle_timer.cc (right):

http://codereview.chromium.org/8806/diff/11/13#newcode11
Line 11: #if defined(OS_LINUX) && 0
Perhaps rather than the 0, just make up a define like "DISABLE_XSS_SUPPORT" and
test that.  Then define it just above here and put the comment about twiddling
it to enable the support above it.