Add support for UserIdleTime to Linux.

Add support for UserIdleTime to Linux.

BUG=32421
TEST=none


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

[Elliot Glaysher, 2010-01-22 00:47:44]

http://codereview.chromium.org/553051/diff/3003/4008
File chrome/browser/sync/engine/idle_query_linux.cc (right):

http://codereview.chromium.org/553051/diff/3003/4008#newcode53
chrome/browser/sync/engine/idle_query_linux.cc:53: &event_base, &error_base)) {
I believe XScreenSaverQueryExtension should be called once instead of on every
query. Read this patch to gaim from the powertop people:
http://www.lesswatts.org/projects/powertop/patches/pidgin-2.0.0.patch

The IdleData() constructor guarantees this was run, right? So I think this call
is redundant?

[akalin, 2010-01-22 00:53:06]

LGTM from sync side.  (I don't know X stuff that well so I'll let erg comment on
that.)

http://codereview.chromium.org/553051/diff/3003/4008
File chrome/browser/sync/engine/idle_query_linux.cc (right):

http://codereview.chromium.org/553051/diff/3003/4008#newcode57
chrome/browser/sync/engine/idle_query_linux.cc:57: return
(idle_data_->mit_info_->idle) / 1000;
no need for parantheses here

http://codereview.chromium.org/553051/diff/3003/4009
File chrome/browser/sync/engine/idle_query_linux.h (right):

http://codereview.chromium.org/553051/diff/3003/4009#newcode21
chrome/browser/sync/engine/idle_query_linux.h:21: IdleData* idle_data_;
scoped_ptr here?

Actually, since IdleData contains nothing but pointers, why don't you just put
all its members into IdleQueryLinux properly forward-declared so you can still
keep the X11 deps in the .cc file.

[Zachary Kuznia, 2010-01-22 01:22:44]

http://codereview.chromium.org/553051/diff/3003/4008
File chrome/browser/sync/engine/idle_query_linux.cc (right):

http://codereview.chromium.org/553051/diff/3003/4008#newcode53
chrome/browser/sync/engine/idle_query_linux.cc:53: &event_base, &error_base)) {
On 2010/01/22 00:47:44, Elliot Glaysher wrote:
> I believe XScreenSaverQueryExtension should be called once instead of on every
> query. Read this patch to gaim from the powertop people:
> http://www.lesswatts.org/projects/powertop/patches/pidgin-2.0.0.patch
> 
> The IdleData() constructor guarantees this was run, right? So I think this
call
> is redundant?

Done.

http://codereview.chromium.org/553051/diff/3003/4008#newcode57
chrome/browser/sync/engine/idle_query_linux.cc:57: return
(idle_data_->mit_info_->idle) / 1000;
On 2010/01/22 00:53:07, akalin wrote:
> no need for parantheses here

Done.

http://codereview.chromium.org/553051/diff/3003/4009
File chrome/browser/sync/engine/idle_query_linux.h (right):

http://codereview.chromium.org/553051/diff/3003/4009#newcode21
chrome/browser/sync/engine/idle_query_linux.h:21: IdleData* idle_data_;
On 2010/01/22 00:53:07, akalin wrote:
> scoped_ptr here?
Switched to scoped_ptr.

> 
> Actually, since IdleData contains nothing but pointers, why don't you just put
> all its members into IdleQueryLinux properly forward-declared so you can still
> keep the X11 deps in the .cc file.

Unfortunately, XScreenSaverInfo is set up in such a way that forward declaring
it doesn't work.

[Elliot Glaysher, 2010-01-22 01:26:05]

X stuff LGTM

[akalin, 2010-01-22 01:56:26]

LGTM after these comments

http://codereview.chromium.org/553051/diff/7001/8006
File chrome/browser/sync/engine/idle_query_linux.cc (right):

http://codereview.chromium.org/553051/diff/7001/8006#newcode34
chrome/browser/sync/engine/idle_query_linux.cc:34: XScreenSaverInfo *mit_info_;
since these variables are public, no need to append an underscore

http://codereview.chromium.org/553051/diff/7001/8006#newcode38
chrome/browser/sync/engine/idle_query_linux.cc:38:
IdleQueryLinux::IdleQueryLinux() {
you can do IdleQueryLinux() : idle_data_(new IdleData()) {} here

http://codereview.chromium.org/553051/diff/7001/8006#newcode50
chrome/browser/sync/engine/idle_query_linux.cc:50:
XScreenSaverQueryInfo(idle_data_->display_,
check return status of this function -- according to the docs if it returns 0,
then it doesn't fill in mit_info_ and so mit_info_->idle has an undefined value

[William Hesse, 2010-01-27 16:17:17]

http://codereview.chromium.org/553051/diff/5008/2023
File chrome/browser/sync/engine/idle_query_linux.cc (right):

http://codereview.chromium.org/553051/diff/5008/2023#newcode7
chrome/browser/sync/engine/idle_query_linux.cc:7: #include
<X11/extensions/scrnsaver.h>
I'm not sure that we have this include file listed as a dependency for Chromium.
 What (Debian or Ubuntu) package is it in, that we should have it? 
x11proto-scrnsaver-dev? Is this listed on
http://code.google.com/p/chromium/wiki/LinuxBuildInstructionsPrerequisites?

[William Hesse, 2010-01-27 16:27:44]

On 2010/01/27 16:17:17, William Hesse wrote:
> http://codereview.chromium.org/553051/diff/5008/2023
> File chrome/browser/sync/engine/idle_query_linux.cc (right):
> 
> http://codereview.chromium.org/553051/diff/5008/2023#newcode7
> chrome/browser/sync/engine/idle_query_linux.cc:7: #include
> <X11/extensions/scrnsaver.h>
> I'm not sure that we have this include file listed as a dependency for
Chromium.
>  What (Debian or Ubuntu) package is it in, that we should have it? 
> x11proto-scrnsaver-dev? Is this listed on
> http://code.google.com/p/chromium/wiki/LinuxBuildInstructionsPrerequisites?

I guess we should add libxss-dev to the prerequisites list.