584660 - Polling in nsIdleService should not be used for Maemo 6 platform

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Jon Hemming]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.11) Gecko/2009060309 Ubuntu/8.04 (hardy) Firefox/3.0.11
Build Identifier: 

If polling is used in nsIdleService, it will cause wake-up between 5s if any idle observer is in idle to check if those idled observers would not need to be idle anymore. These kind of solution can't be used for mobile platform and we just need to be sure that idled observer are changed back to active when idle time ends.

Reproducible: Always

Actual Results:  
5s wake-up when user idles.

Expected Results:  
No wake-ups that are not really needed when user idles.

Comment 1

[Alon Zakai (:azakai)]

I haven't had a chance to look carefully at the idleservice code yet, but is there any reason this should be Maemo 6-specific?

Comment 2

[Alon Zakai (:azakai)]

The Android idleservice in fact does not do polling, possibly because there isn't an API for that there. Hard to say if that causes bugs or not, though, as Fennec-on-Android has not yet been heavily tested. But I can't find any Android bugs about stuff that sounds idle service related.

It might be the case that polling is not terribly important and Maemo can do without it as well. We can test that.

Comment 3

[Jon Hemming]

This is not Maemo 6 specific if polling is worthless on other platforms also. I'm not completely sure about reasons why this kind of system has been implemented in first place, but at least for Maemo 6, I there seems to be no reason to use it. If it is only for checking if idle time has been interrupted between the handling, we should be able to make system where we just make sure that those interruptions are noticed without the polling. At least stopping the polling should be safe, when we already know that we are in idle state and we know that we are going to be notified about returning from idle state. In this case there should be nothing that would prevent us from suspending the polling in between. This should be valid for every platform that can get notified about this.

Comment 4

[Jon Hemming]

Attachment: Doesn't use polling for Maemo 6 platform

This adds two things to repository. First it prevents using of polling for Maemo 6 platform. Basically it will prevent using most of the code in nsIdleServiceQt since most of the code there was useless for Maemo 6. There isn't any screen saver so there is no point of trying to communicate with it, like it is doing at the moment.
 Second thing that this patch does, it adds calling of ResetIdleTimeOut() to nsAppRunner::XRE_main() function, so that nsIdleService would be reseted when starting up, since if that is not done, it seems that nsIdleService wont work as it should. If reset is not called for it, we end up having single shot timer waking up between one second, until the user will interact for the first time and thus cause the ResetIdleTimeOut() to be called for the first time. Also time before first interaction is not counted to be idle time in this case and that doesn't sound right.

Comment 5

[Jon Hemming]

For this to work, ResetIdleTimeOut() will need to be called when returning from inactive state in Maemo 6, since we don't get the first interaction then. Without that the idle time will end only after user has interacted second time with the hardware. This needs to be added when notifications of different states for mobile devices are added. It could be done for example at the same time when we get signal Maemo::QmDisplayState::On in bug 580588.

Comment 6

[Miika Jarvinen]

This seems reasonable to me, but I think that idle timer should only be reseted  if window is currently on foreground.

Comment 7

[Alon Zakai (:azakai)]

Comment on attachment 464369 [details] [diff] [review]
Doesn't use polling for Maemo 6 platform

1. Let's take out the part with ResetIdleTimeOut - we need to investigate and fix that properly. I'll start doing that.

2. Checks specifically for version 6 seem odd to me - shouldn't we check for >= 6, at least - so we don't need to modify a lot of checks when version 7 comes out?

I believe Oleg should review an updated patch, as it'll be just changes in Qt code.

Comment 8

[Jon Hemming]

(In reply to comment #7)
> 1. Let's take out the part with ResetIdleTimeOut - we need to investigate and
> fix that properly. I'll start doing that.
> 
Yes, that is the right way to do it. There are few issues with idle service. One thing to determine should be that when we are going reset the idle time. One solution would be to count showing of browser to user as user interaction. Also not reseting the idle time should not cause 1s timer. Last thing I noticed about reseting the idle time with Qt is that it will be reseted only when button/key is pressed/released, but for example scrolling of the browser wont be counted as user interaction and thus observers will get notified of idle during scrolling. I will create a new bug about this.

> 2. Checks specifically for version 6 seem odd to me - shouldn't we check for >=
> 6, at least - so we don't need to modify a lot of checks when version 7 comes
> out?
> 
I haven't come up yet with any good reason to use polling on any platform so it can be that it could be removed from all of them and make sure that interaction is always reseting the idle time. I haven't really been investigating that though. Also should we have some general check for mobile platforms so that we could use that when dealing with these use-time issues?

> I believe Oleg should review an updated patch, as it'll be just changes in Qt
> code.
Yes, sorry about this. I should have put this only to feedback and I will put the next patch to review for Oleg.

Comment 9

[Alon Zakai (:azakai)]

> Also should we have some general check for mobile platforms so that we
could use that when dealing with these use-time issues?

We probably should at some point. But it's just 2 mobile platforms so far, so not an urgent issue. Also we might do some of this sort of thing through preferences, and set those in Fennec's prefs for all mobile platforms.

Comment 10

[Jon Hemming]

Attachment: Xulrunner patch v3

Polling is not used if MOZ_PLATFORM_MAEMO > 5.

Comment 11

[Oleg Romashin (:romaxa)]

Comment on attachment 471824 [details] [diff] [review]
Xulrunner patch v3

I don't understand why you are doing ifdefs this way.. ?
should it be something like 

>+#if (MOZ_PLATFORM_MAEMO < 6) or  <= 5
>     // This will leak - See comments in ~nsIdleServiceQt().
>     PRLibrary* xsslib = PR_LoadLibrary("libXss.so.1");

?

Comment 12

[Oleg Romashin (:romaxa)]

Also I think this idle pooling is bad for maemo5 too.... I would suggest just disable this X-implementation pooling for mobile completely.

The same way as it disabled for WIN_CE
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsIdleServiceWin.cpp#67

Comment 13

[Jon Hemming]

I used ifdefs that way since, it was just the first safe way for me that came to my mind. I didn't know what happens if MOZ_PLATFORM_MAEMO is undefined so idea was that if (MOZ_PLATFORM_MAEMO < 6) would give false it wouldn't necessary mean that MOZ_PLATFORM_MAEMO > 5. That is why I used:
#if (MOZ_PLATFORM_MAEMO > 5)
#else
...
#endif
which says exactly what I wanted. Indeed polling is not good for any mobile platform, so I will change that to include all Maemo platforms.

Comment 14

[Oleg Romashin (:romaxa)]

> which says exactly what I wanted. Indeed polling is not good for any mobile
> platform, so I will change that to include all Maemo platforms.
yep, that make more sense for me

Comment 15

[Jon Hemming]

Attachment: Xulrunner patch v4

used ifndef and ifdef

Comment 16

[Oleg Romashin (:romaxa)]

Comment on attachment 471847 [details] [diff] [review]
Xulrunner patch v4

Good.

I think the same thing need to be done for mobile GTK, nsIdleServiceGTK.cpp

Comment 17

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/3b8cd4e4312d