475361 - Need idle service on windows mobile

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Doug Turner (:dougt)]

We have disabled it:

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/Makefile.in#109

and we see lots of assertions:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040154: file c:/builds/mobile/mozilla-central/toolkit/components/places/src/nsNavHistory.cpp, line 5196

Comment 1

[Doug Turner (:dougt)]

http://msdn.microsoft.com/en-us/library/aa915334.aspx

Comment 2

[John Wolfe (:wolfe)]

The GetIdleTime() API measures the amount of time the processor has been idle, not the amount of time since user input.

Since WinMobile devices have all sorts of timer routines going off every second, the GetIdleTime() return value is never very big at all.

No, we need a different approach for setting up an idle timer.

I have been awaiting our shipment of WinMobile alpha in order to debug my code for this bug.

Comment 3

[Doug Turner (:dougt)]

thanks for the clarification.

Comment 4

[John Wolfe (:wolfe)]

Attachment: v1.0 patch

Adds a new nsIIdleServiceWinCE service, and sub-classes the existing nsIdleTimeWin class.  Adds a single function into nsWindow.cpp to reset "idleTime" when user interaction happens to a top-level window (mouse move, key press).

Currently also resets idleTime for Input Method Editor (soft keypad) events.

Comment 5

[Doug Turner (:dougt)]

wolfe, did you investigate using:

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#7990

might be a challenge to access it from the idle service implementation, but it is cleaner than having to have a separate interface and all of the gunk required to support this.

Comment 6

[Stuart Parmenter]

Comment on attachment 362007 [details] [diff] [review]
v1.0 patch

get rid of your printfs

I don't really see any reason for ModifyLastInputTime() and #defining it -- the function isn't very big

safe_compare should have a better name, and macros should be all caps.

you should have [e-mail] review the rest of the patch.

Comment 7

[John Wolfe (:wolfe)]

Attachment: v2.0 patch - greatly simplified

Greatly simplified patch that should have no effect outside of WINCE builds, and only adds one global variable to WINCE builds.

The global variable was already around as a static global variable inside nsWindow.cpp

No need for a new service.  No need for subclassing nsIdleServiceWin.

Comment 8

[Ere Maijala (slow)]

Comment on attachment 366497 [details] [diff] [review]
v2.0 patch - greatly simplified

The non-WinCE part is broken. There's no lastInputTime. Did you mean to make it:

  *aTimeDiff = SAFE_COMPARE_EVEN_WITH_WRAPPING(GetTickCount(), inputInfo.dwTime);

If so, r=me with that fixed.

Comment 9

[John Wolfe (:wolfe)]

Attachment: v2.1 patch - typo fixed

Fixes typo to nsIdleServiceWin.cpp source file patch - Ere was right.

This patch is only a small, requested delta from the patch that makes this patch r=Ere

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/0832dd8ea2b7

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: A patch

Is there any reason not to build nsIdleServiceWin.cpp?

Comment 12

[Doug Turner (:dougt)]

wolfe, how could this have been tested?  ere, should we back this out?

Comment 13

[John Wolfe (:wolfe)]

I must have gotten the Makefile change imbedded into another patch, and not pulled a completely new tree to test the patch.  

This was in the middle of trying to get the testbed working, and I got lost walking through the architecture problems.

This code was a small delta off what I had originally tested - but I will run through the testing again today (with nsIdleServiceWin.cpp building for WinCE as per Hiroyuki's patch) to double-check.

I will post another comment when I have finished testing.

Comment 14

[Ere Maijala (slow)]

Please back out. And please don't check in without super review.

Comment 15

[Doug Turner (:dougt)]

agreed.

http://hg.mozilla.org/mozilla-central/rev/87e1c6919116

Comment 16

[John Wolfe (:wolfe)]

Attachment: v3.0 patch

This new patch superceeds the older patches.  

I have also run through a day of testing of this patch, and see where the code depending upon the idleService properly calls into the idleService.

The idleTime returned uses the (formerly) static nsWindow.cpp variable gLastInputEventTime since the APO GetLastInputInfo() is not available on WinCE.

Some things added to the v2.1 patch: 

* Always compile the nsIdleServiceWin.cpp source file when building for WinCE

* Always generate the nsIdleServiceWin Generic Factory Constructor when building for WinCE

* Always add the NS_IDLE_SERVICE_CID and service information to the listing of services when building for WinCE

* Initialize the gLastInputEventTime global variable when a window is created - just in case the user starts XULRunner but does not produce and mouse or character events, an unlikely scenario.  In this case we simply say that Window creation marks the last input event time.

Apologies for the poorly tested v2.1 patch.

Comment 17

[Ere Maijala (slow)]

Comment on attachment 371717 [details] [diff] [review]
v3.0 patch

Just one nit: 

+  // NOTE: nowTime is not necessarily equivalent to GetTickCount() return value
+  //       we need to compare apples to apples - hence the nowTime variable
+    PRUint32 nowTime = PR_IntervalToMicroseconds(PR_IntervalNow());

I'd indent the comments four characters too.

Please request super-review now.

Comment 18

[John Wolfe (:wolfe)]

Attachment: v3.1 patch - fixes comment indentation

This patch fixes the indentation spacing as requested by Ere Maijala.

The previous attachment was r+ere

Comment 19

[Stuart Parmenter]

Comment on attachment 372348 [details] [diff] [review]
v3.1 patch - fixes comment indentation

I would leave gLastInputEventTime as static and use nsWindow::GetLastInputEventTime in the idle service.

Comment 20

[John Wolfe (:wolfe)]

Stuart changed his sr- to sr+ after some discussion.

changeset:   27326:ea400876d06c
user:        John Wolfe <[e-mail]>
date:        Tue Apr 14 14:23:01 2009 -0400
summary:     Bug 475361 - Need idle service on windows mobile - r+emaijala, sr+pavlov

Comment 21

[Doug Turner (:dougt)]

clearing wanted1.9.1 flag.  we need this for fennec.

Comment 22

[Doug Turner (:dougt)]

clearing wanted flag;