Closed Bug 475361 Opened 11 years ago Closed 11 years ago

Need idle service on windows mobile

Categories

(Core :: Widget: Win32, defect)

ARM
Windows CE
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 1.0a1-wm+ ---

People

(Reporter: dougt, Assigned: dougt)

References

Details

(Keywords: mobile, Whiteboard: [wm-a1-b+])

Attachments

(1 file, 5 obsolete files)

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
Flags: wanted1.9.1?
Flags: blocking-fennec1.0+
Whiteboard: [wm-a1-b+]
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.
thanks for the clarification.
Attached patch v1.0 patch (obsolete) — Splinter Review
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.
Attachment #362007 - Flags: review?(pavlov)
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.
tracking-fennec: --- → 1.0a1-wm+
Flags: blocking-fennec1.0+
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 emaijala@kolumbus.fi review the rest of the patch.
Attachment #362007 - Flags: review?(pavlov) → review-
Attached patch v2.0 patch - greatly simplified (obsolete) — Splinter Review
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.
Attachment #362007 - Attachment is obsolete: true
Attachment #366497 - Flags: review?(pavlov)
Attachment #366497 - Flags: review?(pavlov) → review?(emaijala)
Status: NEW → ASSIGNED
Keywords: mobile
OS: Mac OS X → Windows CE
Hardware: x86 → ARM
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.
Attachment #366497 - Flags: review?(emaijala) → review-
Attached patch v2.1 patch - typo fixed (obsolete) — Splinter Review
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
Attachment #366497 - Attachment is obsolete: true
Attachment #368718 - Flags: review+
pushed http://hg.mozilla.org/mozilla-central/rev/0832dd8ea2b7
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attached patch A patch (obsolete) — Splinter Review
Is there any reason not to build nsIdleServiceWin.cpp?
wolfe, how could this have been tested?  ere, should we back this out?
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.
Please back out. And please don't check in without super review.
agreed.

http://hg.mozilla.org/mozilla-central/rev/87e1c6919116
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch v3.0 patch (obsolete) — Splinter Review
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.
Attachment #368718 - Attachment is obsolete: true
Attachment #370583 - Attachment is obsolete: true
Attachment #371717 - Flags: review?(emaijala)
Attachment #371717 - Flags: review?(emaijala) → review+
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.
This patch fixes the indentation spacing as requested by Ere Maijala.

The previous attachment was r+ere
Attachment #371717 - Attachment is obsolete: true
Attachment #372348 - Flags: superreview?(pavlov)
Attachment #372348 - Flags: review+
Attachment #372348 - Attachment is patch: true
Attachment #372348 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #372348 - Flags: superreview?(pavlov) → superreview-
Attachment #372348 - Flags: superreview- → superreview+
Stuart changed his sr- to sr+ after some discussion.

changeset:   27326:ea400876d06c
user:        John Wolfe <wolfe@lobo.us>
date:        Tue Apr 14 14:23:01 2009 -0400
summary:     Bug 475361 - Need idle service on windows mobile - r+emaijala, sr+pavlov
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Depends on: 489815
clearing wanted1.9.1 flag.  we need this for fennec.
Flags: wanted1.9.1? → wanted-fennec1.0?
clearing wanted flag;
Flags: wanted-fennec1.0?
You need to log in before you can comment on or make changes to this bug.