366337 - implement nsIIdleService on Windows

Status: RESOLVED FIXED

(Core :: Widget: Win32, enhancement)

Description

[:Gijs (he/him)]

It'd be nice to have an implementation for this on Windows as well. I'll look into it myself when I can. (Hopefully Soon)

Comment 1

[:Gijs (he/him)]

Attachment: Patch

I have way too much time on my hands. Anyway, implementation using GetLastInputInfo, which is available on Win2k and up, which is what trunk is catering for I believe, but it's not there on Win CE, so the makefile and interface stuff is in ifdef's to make sure we don't break builds there.

Comment 2

[[e-mail]]

Comment on attachment 251058 [details] [diff] [review]
Patch

sr(!)=me with these nits addressed.

>+#include "nsIServiceManager.h"
>+#include "nsDebug.h"
Not used?

>+nsIdleServiceWin::nsIdleServiceWin()
>+{}
>+
>+nsIdleServiceWin::~nsIdleServiceWin()
>+{}
Not sure if there's any point declaring these if they don't do anything.

>+    memset(&inputInfo, 0, sizeof(inputInfo));
Unnecessary; you only need to set cbSize.

>+    DWORD ticks = GetTickCount();
There's no point calling this before GetLastInputInfo. Also it's only used once so you might as well inline it.

>+    PRBool rv = GetLastInputInfo(&inputInfo);
Nit: module style is to use :: on globals. Also, it's declared to return BOOL, not PRBool. Or just write if (!::GetLastInputInfo(&inputInfo))

Comment 3

[Nickolay_Ponomarev]

Comment on attachment 251058 [details] [diff] [review]
Patch

Alert! Windows line endings!

>+nsIdleServiceWin::GetIdleTime(PRUint32 *aTimeDiff)
>+{
>+    LASTINPUTINFO inputInfo;
>+    memset(&inputInfo, 0, sizeof(inputInfo));
>+    inputInfo.cbSize = sizeof(inputInfo);
>+    DWORD ticks = GetTickCount();
>+    PRBool rv = GetLastInputInfo(&inputInfo);
>+    if (rv == 0)
>+        return NS_ERROR_FAILURE;
>+    *aTimeDiff = ticks - inputInfo.dwTime;
>+    return NS_OK;

Nit of the day: so if I'm away for a long period of time, the idle observers will receive a 'back' notification once in 50 days? (What about Linux implementation? Perhaps the wrapping-around behavior of |attribute idleTime| should be documented in the IDL?)

Also rv == 0 is equivalent to !rv.

Comment 4

[Nickolay_Ponomarev]

Comment on attachment 251058 [details] [diff] [review]
Patch

bah, stupid bugzilla. Restoring Neil's r+.

Comment 5

[:Gijs (he/him)]

Attachment: Patch v2

Carrying over sr+ from Neil.

This should have the nits you pointed out fixed. I've also re-saved the diff with just unix line-ends. Looks like my msys cvs diff somehow prefers those. Suggestions on how to teach it not to are welcome. There's an xxx from me wrt the 49.7 days thing.

Comment 6

[Ere Maijala (slow)]

Comment on attachment 251127 [details] [diff] [review]
Patch v2

Well, in my opinion you can remove the XXX comment. For this to actually cause any problems you need to have the system idle for more than 49.7 days. GetTickCount() simply wrapping doesn't cause trouble. See for example <http://www.thetaeng.com/TimerWrap.htm> for an article on it, and it's also mentioned in MSDN GetTickCount() docs.

r=emaijala

Comment 7

[:Gijs (he/him)]

Ugh, I forgot to remove the XXX comment the first round, so there's two checkins for this bug now. Sorry.

Checking in build/nsWinWidgetFactory.cpp;
new revision: 1.74; previous revision: 1.73
Checking in windows/Makefile.in;
new revision: 3.46; previous revision: 3.45
Checking in windows/nsIdleServiceWin.cpp;
initial revision: 3.1
Checking in windows/nsIdleServiceWin.h;
initial revision: 3.1

Followed by:

Checking in windows/nsIdleServiceWin.cpp;
new revision: 3.2; previous revision: 3.1

Comment 8

[:Gijs (he/him)]

So, this broke a bundle of tinderboxen. The Thunderbird box didn't seem to know about LASTINPUTINFO. I really don't have a clue what's up there. Robert Sayre told me he got the same error as the Fx and SeaMonkey boxen, and fixed it by adding an empty constructor. I backed out before he told me that. I'll add a patch here after I've tested it myself.

Comment 9

[:Gijs (he/him)]

Turns out the thunderbird bustage is to blame on a flag causing it to try to build in a win95 compatible way, which breaks this patch because the API didn't exist before win2k. Marking the bug about changing that flag as blocking this one.

Comment 10

[:Gijs (he/him)]

Attachment: Better patch

Just removing the declarations from the .h file works too. At least, it compiles and runs and (manually) tests fine. The thunderbird tinderbox fixed has a patch and review, it just needs checkin and some looking after, or so I hope :-)

Comment 11

[[e-mail]]

Comment on attachment 252051 [details] [diff] [review]
Better patch

>+#include "nsIServiceManager.h"
Nit: I still don't see where you're using this.

Comment 12

[Ere Maijala (slow)]

Comment on attachment 252051 [details] [diff] [review]
Better patch

Check neil's comment. 
r=emaijala

Comment 13

[:Gijs (he/him)]

Checking in src/build/nsWinWidgetFactory.cpp;
/cvsroot/mozilla/widget/src/build/nsWinWidgetFactory.cpp,v  <--  nsWinWidgetFactory.cpp
new revision: 1.76; previous revision: 1.75
done
Checking in src/windows/Makefile.in;
/cvsroot/mozilla/widget/src/windows/Makefile.in,v  <--  Makefile.in
new revision: 3.48; previous revision: 3.47
done
Checking in src/windows/nsIdleServiceWin.cpp;
/cvsroot/mozilla/widget/src/windows/nsIdleServiceWin.cpp,v  <--  nsIdleServiceWin.cpp
new revision: 3.5; previous revision: 3.4
done
Checking in src/windows/nsIdleServiceWin.h;
/cvsroot/mozilla/widget/src/windows/nsIdleServiceWin.h,v  <--  nsIdleServiceWin.h
new revision: 3.3; previous revision: 3.2
done


(I've removed the include mentioned in comment #11)
Tinderboxen are still a lovely shade of green, so marking this FIXED.