Idle service does not work on Qt on mobile platforms

RESOLVED FIXED

Status

Core Graveyard
Widget: Qt
RESOLVED FIXED
7 years ago
10 months ago

People

(Reporter: dougt, Assigned: MikeK)

Tracking

(Depends on: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Reporter)

Description

7 years ago
because some installations of Qt do not have screensaver support, we need to do something similar that we did for windows mobile and Gtk2.
(Reporter)

Updated

7 years ago
Assignee: nobody → mkristoffersen
Any patch? I know it exists ;)
(Assignee)

Comment 2

7 years ago
Created attachment 425203 [details] [diff] [review]
This version only works on QT builds - will break other builds, compile time or runtime
(Assignee)

Comment 3

7 years ago
Created attachment 426597 [details] [diff] [review]
Now with cross-platform support

Waiting for the try server to come up with some results, will post the result when I have it.
Attachment #425203 - Attachment is obsolete: true
(Assignee)

Comment 4

7 years ago
Oh, btw.  can someone help check/test this patch on WinMo?  (Especially that it doesn't introduce more checker boarding)  (OS X test will also be welcome, but might be harder)
(Assignee)

Updated

7 years ago
Attachment #426597 - Flags: review?(mozbugz)
(Reporter)

Comment 5

7 years ago
Comment on attachment 426597 [details] [diff] [review]
Now with cross-platform support

sdwilsh, can you take a first look?
Attachment #426597 - Flags: review?(dougt) → review?(sdwilsh)
Comment on attachment 426597 [details] [diff] [review]
Now with cross-platform support

A bunch of this patch didn't apply cleanly, making it difficult to review.  Please address these comments, and then update it to tip of mozilla-central and I'll take a look again.  Comments with context can be found at http://reviews.visophyte.org/r/426597/

on file: widget/public/nsIIdleService.idl line 97
>     /**
>      * Resets the idle timeout, telling the system that activity has happend
>      * @note
>      * This won't tell the underlying OS that activity has happend, so it won't
>      * prevent screensavers etc. from starting
>      */
>     void resetIdleTimeOut();

This should probably go on a new interface that is only QIable from binary
code and not on this.  We don't want anyone to be able to do this really.


on file: widget/src/cocoa/nsIdleServiceX.h line 47
>   PRBool PollIdleTime(PRUint32* idleTime);

s/PRBool/bool/ please


on file: widget/src/gtk2/nsIdleServiceGTK.h line 25
>  *  Gijs Kruitbosch <gijskruitbosch@gmail.com>

er?


on file: widget/src/gtk2/nsIdleServiceGTK.cpp line 25
>  *  Gijs Kruitbosch <gijskruitbosch@gmail.com>

?


on file: widget/src/xpwidgets/nsIdleService.h line 97
>     // If you overwrite ResetIdleTimeout, you MUST call this implementation, or
>     // there is a risk that the system won't wake up again, after user activity
>     // resumes

I don't see why anyone would want to override this, and we could always add
hooks for stub classes.  We should use the NS_FINAL (or something like that)
attribute so static analysis can check this.


on file: widget/src/xpwidgets/nsIdleService.h line 106
>     // If there is a platform specific function to poll the system idel time
>     // then that must be returned in this function, and the function MUST return
>     // PR_TRUE, otherwise then the function should be left unimplemented or made
>     // to return PR_FALSE (this can also be used for systems where it depends on
>     // the configuration of the system if the idle time can be determined)
>     //
>     // NOTE: The time returned by this function can be different than the one
>     // returned by GetIdleTime, as that is corrected by any calls to
>     // ResetIdleTimeOut(), unless you overwrite that function too...
>     //
>     // The time returned in idleTime is meassured in ms

proper java-doc style please


on file: widget/src/xpwidgets/nsIdleService.h line None

s/PRBool/bool/


on file: widget/src/xpwidgets/nsIdleService.h line None

s/PRBool/bool/


on file: widget/src/xpwidgets/nsIdleService.h line None

Proper java-doc style please.  Also, indicating which timer would be useful
(we have daily, idle, etc)


on file: widget/src/xpwidgets/nsIdleService.h line None

Proper java-doc style please


on file: widget/src/xpwidgets/nsIdleService.h line 132
>     // Place to hold the timer

Timer for what?


on file: widget/src/xpwidgets/nsIdleService.h line 138
>     // Contains the time of the last idle reset or 0 there haven't been a reset
>     // Time is kept in seconds
>     PRUint32 mLastIdleReset;
> 
>     // The time since the last handled activity (which might be different than
>     // mLastIdleReset, since the activity that reset the idle timer could just
>     // have happend, and not handled yet)
>     // Time is kept in seconds since the epoch at midnight, January 1, 1970 UTC
>     PRUint32 mLastHandledActivity;

We usually use this style for multiline comments:
/**
 * comment
 */


on file: widget/src/xpwidgets/nsIdleService.cpp line 1
> /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

You are making this file inconsistent by making this change.  Please keep it
at 4.


on file: widget/src/xpwidgets/nsIdleService.cpp line 81
>   nsCOMPtr<nsIObserverService> observerService =
>                                do_GetService("@mozilla.org/observer-service;1");

only indent one level.  this indentation looks odd


on file: widget/src/xpwidgets/nsIdleService.cpp line None

nit: closing parent on a new line


on file: widget/src/xpwidgets/nsIdleService.cpp line 144
> NS_IMETHODIMP nsIdleService::AddIdleObserver(nsIObserver* aObserver,
>                                              PRUint32 aIdleTime)

NS_IMETHODIMP should be on its own line (in general, commenting only once)
Attachment #426597 - Flags: review?(sdwilsh) → review-
(Assignee)

Comment 7

7 years ago
(In reply to comment #6)
> (From update of attachment 426597 [details] [diff] [review])
> A bunch of this patch didn't apply cleanly, making it difficult to review. 
> Please address these comments, and then update it to tip of mozilla-central and
> I'll take a look again.  Comments with context can be found at
> http://reviews.visophyte.org/r/426597/

Yeah, that tends to happen when patches are laying around for a month after the review has been requested :)  (not your fault, I blame the system) - I have merged it to trunk, will post in a minute.

> This should probably go on a new interface that is only QIable from binary
> code and not on this.  We don't want anyone to be able to do this really.

Nah, I guess it could be miss-used by bad guys, I'll investigate how to prevent the exposure of an interface.


> 
> on file: widget/src/cocoa/nsIdleServiceX.h line 47
> >   PRBool PollIdleTime(PRUint32* idleTime);
> 
> s/PRBool/bool/ please

We can use bool, I forgot that - great, this is one change I'll be more than happy to apply to the patch :)
 
> 
> on file: widget/src/gtk2/nsIdleServiceGTK.h line 25
> >  *  Gijs Kruitbosch <gijskruitbosch@gmail.com>
> 
> er?

That comment/question, didn't make much sense to me ?

> on file: widget/src/xpwidgets/nsIdleService.h line 97
> >     // If you overwrite ResetIdleTimeout, you MUST call this implementation, or
> >     // there is a risk that the system won't wake up again, after user activity
> >     // resumes
> 
> I don't see why anyone would want to override this, and we could always add
> hooks for stub classes.  We should use the NS_FINAL (or something like that)
> attribute so static analysis can check this.

It could be relevant to override if the underlying system/OS have some mechanism that should be triggered in the case of software generated user activity.

> 
> proper java-doc style please

np

> 
> on file: widget/src/xpwidgets/nsIdleService.h line None
> 
> Proper java-doc style please.  Also, indicating which timer would be useful
> (we have daily, idle, etc)

java-doc, np - I don't understand the second half of the comment?

> We usually use this style for multiline comments:
> /**
>  * comment
>  */

I'll change it, in this patch

> 
> on file: widget/src/xpwidgets/nsIdleService.cpp line 1
> > /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> 
> You are making this file inconsistent by making this change.  Please keep it
> at 4.

As this file is basically being rewritten, I was under the impression that we should move it to a 2 space indentation, to comply with the style guide:

"The following norms should be followed for new code, and for Tower of Babel code that needs cleanup."

But I rather change it to 4 space indentation than getting into a discussion about it?

> on file: widget/src/xpwidgets/nsIdleService.cpp line 81
> >   nsCOMPtr<nsIObserverService> observerService =
> >                                do_GetService("@mozilla.org/observer-service;1");
> 
> only indent one level.  this indentation looks odd

ok, I suppose you mean, like:

  // Notify anyone who cares
  nsCOMPtr<nsIObserverService> observerService =
    do_GetService("@mozilla.org/observer-service;1");
(Assignee)

Comment 8

7 years ago
Created attachment 431352 [details] [diff] [review]
Patch updated to trunk - review comments not addressed (yet)

Note that in order to build qt on the current trunk, you also need to apply the patch to bug 551149.

Patch with the review comments addressed will follow.
Attachment #426597 - Attachment is obsolete: true
(In reply to comment #7)
> Nah, I guess it could be miss-used by bad guys, I'll investigate how to prevent
> the exposure of an interface.
You'll want to create a UID for the native class that you can QI to (it's a safe way to cast).  It's pretty simple; here's an interface that I recently did this too:
http://mxr.mozilla.org/mozilla-central/source/docshell/base/IHistory.h

> We can use bool, I forgot that - great, this is one change I'll be more than
> happy to apply to the patch :)
Just don't use it in places where we can cross xpconnect.  Those places we can't do it with.

> That comment/question, didn't make much sense to me ?
Diff was showing me a change on that line, but I don't see one, so you changed some invisible character.

> It could be relevant to override if the underlying system/OS have some
> mechanism that should be triggered in the case of software generated user
> activity.
Sure, but we can always add hooks for it in the future.  For now, we should protect it (it also makes sure invariants are checked when/if someone does need it)

> java-doc, np - I don't understand the second half of the comment?
"timer" is vague; I'd like a slightly more detailed description (looking to help with other people being able to jump into this code)

> As this file is basically being rewritten, I was under the impression that we
> should move it to a 2 space indentation, to comply with the style guide:
I'm fine with it if you change the whole file.  I saw parts that had four spaces still.  You also forgot to update the vim modeline.  If you do decide to update the whole file, please do it in a separate patch (but in this bug is fine).  No reason to make the actual changes patch bigger with whitespace changes.

> ok, I suppose you mean, like:
>   // Notify anyone who cares
>   nsCOMPtr<nsIObserverService> observerService =
>     do_GetService("@mozilla.org/observer-service;1");
Bingo.
(Assignee)

Comment 10

7 years ago
Created attachment 432922 [details] [diff] [review]
Review comments addressed, updated to trunk as of this morning, but failing mochitest

See this as a progress report, I'm trying to figure out why its currently failing some of the test on the tryserver...

Fits:

changeset:   39479:050887c64183
tag:         tip
user:        Marco Bonardo <mbonardo@mozilla.com>
date:        Tue Mar 16 02:02:50 2010 +0100
summary:     Bug 552386 - test_history_sidebar.js fails between midnight and 2:00am with new DST settings, r=dietrich a=dholbert for CLOSED TREE
Attachment #431352 - Attachment is obsolete: true
(Assignee)

Comment 11

7 years ago
Created attachment 433425 [details] [diff] [review]
No more known issues
Attachment #432922 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #433425 - Flags: review?(sdwilsh)

Updated

7 years ago
Attachment #433425 - Flags: review?(sdwilsh) → review+
Comment on attachment 433425 [details] [diff] [review]
No more known issues

http://reviews.visophyte.org/r/433425/
on file: widget/src/gtk2/nsWindow.cpp line 264
> UpdateLastInputEventTime() {

nit: brace on new line


on file: widget/src/qt/nsWindow.cpp line 1261
>     // The user has done something

nit: use punctuation too please! (in all places)


on file: widget/src/qt/nsWindow.cpp line 2526
>   nsCOMPtr<nsIdleService> idleService =
>    do_GetService("@mozilla.org/widget/idleservice;1");

you probably want to cache this and not do a do_GetService every time we call
this


on file: widget/src/xpwidgets/nsIdleService.h line 83
>    * @param aIdleService pointer to the idle service

nit: description goes on line after aIdleService, lined up with aIdleService


on file: widget/src/xpwidgets/nsIdleService.h line 95
>    * NOTE: This is a normal pointer, or the idle service could keep it self
>    * alive

nit: @note


on file: widget/src/xpwidgets/nsIdleService.h line 139
>    * @param idleTime The idle time in ms

nit: aIdleTime
nit: The on next line


on file: widget/src/xpwidgets/nsIdleService.h line 143
>    * @note
>    * The time returned by this function can be different than the one
>    * returned by GetIdleTime, as that is corrected by any calls to
>    * ResetIdleTimeOut(), unless you overwrite that function too...

usually we do it like this:
@note The time...
      ....
      ....


on file: widget/src/xpwidgets/nsIdleService.h line 169
>    * @param aDelay The time in seconds that should pass before the next
>    *               timeout

nit: new line for description


on file: widget/src/xpwidgets/nsIdleService.h line 208
>    * <p>

what is the <p>?


on file: widget/src/xpwidgets/nsIdleService.cpp line 132
>     MAX_IDLE_POLL_INTERVAL);

nit: closing paren on new line, no indent


on file: widget/src/xpwidgets/nsIdleService.cpp line 233
>   PRUint32 timeSinceReset = PR_IntervalToSeconds(PR_IntervalNow()) -
>                                                                 mLastIdleReset;

weird indent here
also, I wonder if we should be using TimeStamp now


on file: widget/src/xpwidgets/nsIdleService.cpp line 244
>   *idleTime = PR_MIN(timeSinceReset * 1000, polledIdleTimeMS);

use NS_MIN, not PR_MIN


r=sdwilsh
(Assignee)

Comment 13

7 years ago
(In reply to comment #12)
> (From update of attachment 433425 [details] [diff] [review])
> http://reviews.visophyte.org/r/433425/
<SNIP>
> on file: widget/src/xpwidgets/nsIdleService.h line 208
> >    * <p>
> 
> what is the <p>?

Paragraph break, according to http://java.sun.com/j2se/javadoc/writingdoccomments/ that is
refered to in our Coding Style ( https://developer.mozilla.org/En/Developer_Guide/Coding_Style )

> 
> 
> on file: widget/src/xpwidgets/nsIdleService.cpp line 132
> >     MAX_IDLE_POLL_INTERVAL);
> 
> nit: closing paren on new line, no indent

Like this?:

  static_cast<nsIdleServiceDaily*>(aClosure)->mIdleService->AddIdleObserver(
    static_cast<nsIdleServiceDaily*>(aClosure),
    MAX_IDLE_POLL_INTERVAL
  );

> on file: widget/src/xpwidgets/nsIdleService.cpp line 233
> >   PRUint32 timeSinceReset = PR_IntervalToSeconds(PR_IntervalNow()) -
> >                                                                 mLastIdleReset;
> 
> weird indent here
> also, I wonder if we should be using TimeStamp now

TimeStamp ?

> r=sdwilsh

Thanks for the review, updated patch pending...
(In reply to comment #13)
> Paragraph break, according to
> http://java.sun.com/j2se/javadoc/writingdoccomments/ that is
> refered to in our Coding Style (
> https://developer.mozilla.org/En/Developer_Guide/Coding_Style )
Neat.  I have never seen that before.

>   static_cast<nsIdleServiceDaily*>(aClosure)->mIdleService->AddIdleObserver(
>     static_cast<nsIdleServiceDaily*>(aClosure),
>     MAX_IDLE_POLL_INTERVAL
>   );
Yes!  Although it might also make sense to make a temporary here since you static_cast the same thing to the same type twice.

> TimeStamp ?
http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/TimeStamp.h
the NSPR stuff has some gotchas so roc wrote a class that hides those gotchas.  We are supposed to try to use this when doing time difference calculations, so I think we should probably be using it here as well.
(Assignee)

Comment 15

7 years ago
> >   static_cast<nsIdleServiceDaily*>(aClosure)->mIdleService->AddIdleObserver(
> >     static_cast<nsIdleServiceDaily*>(aClosure),
> >     MAX_IDLE_POLL_INTERVAL
> >   );
> Yes!  Although it might also make sense to make a temporary here since you
> static_cast the same thing to the same type twice.

And presto, it fitted on one line.

> 
> > TimeStamp ?
> http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/TimeStamp.h
> the NSPR stuff has some gotchas so roc wrote a class that hides those gotchas. 
> We are supposed to try to use this when doing time difference calculations, so
> I think we should probably be using it here as well.

I'll have a look at that interface
(Assignee)

Comment 16

7 years ago
Created attachment 435273 [details] [diff] [review]
Updated patch

The movement of the code to use the "TimeStamp" interface will be done seperately, other issues should be addressed.
Attachment #433425 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
(Assignee)

Comment 17

7 years ago
Created bug 555313 to deal with the TimeStamp update.
Depends on: 555313
(Assignee)

Comment 18

7 years ago
Created attachment 436015 [details] [diff] [review]
Updated to trunk

Fits:

changeset:   40013:3d16328f87ed
tag:         tip
user:        Brad Lassey <blassey@mozilla.com>
date:        Tue Mar 30 15:23:44 2010 -0400
summary:     update nspr to NSPR_HEAD_20100330 to pick up support for android r=ted
Attachment #435273 - Attachment is obsolete: true
(Assignee)

Comment 19

7 years ago
Pushed as: http://hg.mozilla.org/mozilla-central/rev/df9ac939b708
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Depends on: 556260
Backed out because it regressed Ts (all platforms) and Ts shutdown (all platforms),
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 21

7 years ago
Created attachment 436241 [details] [diff] [review]
Includes the patch from bug 556260
Attachment #436015 - Attachment is obsolete: true
(Assignee)

Comment 22

7 years ago
Try server builds at: /tryserver-builds/mkristoffersen@mozilla.com-IdlePatchTake2
(Assignee)

Updated

7 years ago
Attachment #436241 - Flags: review?(sdwilsh)
How about I r+ the patch in bug 556260 since that has a smaller, and simpler patch.  Feel free to merge them when you land next.
(Assignee)

Comment 24

7 years ago
Thats much more easy for everyone :)
(Assignee)

Comment 25

7 years ago
Here are some of talos results from the try sever
The patch had two runs, one before the orginal patch was backed out, and one after (where it was merged with the original patch)
"Others" mean other try server runs that are unrelated to this patch

MacOSX Darwin 9.0.0 try talos dirty:
ts_cold_generated_min
---------------------
Patch:
12473.74 / 11223,16
Others:
12397.26 / 10706.05 / 11541.0 / 10370.47

ts_places_generated_max
-----------------------
Patch:
819.42 / 835.58

Others:
831.11 / 938.74 / 961.84 / 926.53

WINNT 5.1 try talos dirty
*************************
ts_cold_generated_min
---------------------
Patch: 
513.21 / 514.74
Others:
536.89 / 516.26 / 513.63 / 513.05 / 515.05

ts_places_generated_max
-----------------------
Patch:
515.47 / 517.84

Others:
515.68 / 560.0 / 515.37 / 513.42
(Assignee)

Comment 26

7 years ago
(In reply to comment #25)
> ts_cold_generated_min
should be read as ts_cold_generated_max
(Assignee)

Updated

7 years ago
Attachment #436241 - Flags: review?(sdwilsh)
(Assignee)

Comment 27

7 years ago
Ok, might be a little complicated - the current patch is the updated r+ one patched with the r+ patch from bug 556260.

In other words, this is the patch as it should have been and sdwilsh (the reviewer) has OK'ed that the two patches was merged and submitted as one - any volunteers to push it are welcome :)
Keywords: checkin-needed
(Assignee)

Comment 28

7 years ago
Created attachment 438834 [details] [diff] [review]
Updated to trunk

Fits:

changeset:   40733:caa4eb12ad20
tag:         tip
user:        Robert Longson <longsonr@gmail.com>
date:        Tue Apr 13 09:58:09 2010 +0100
summary:     Bug 456286 - support altGlyph elements as tspans. r=roc
Attachment #436241 - Attachment is obsolete: true
(Assignee)

Comment 29

7 years ago
Removed the chekin-needed flag as I haven't checked if the updating to trunk changed the performance of the previous patch - try server run seems ok.
Keywords: checkin-needed
(Assignee)

Comment 30

7 years ago
Created attachment 439330 [details] [diff] [review]
Yet again updated to trunk
Attachment #438834 - Attachment is obsolete: true
(Reporter)

Comment 31

7 years ago
pushed:
  http://hg.mozilla.org/mozilla-central/rev/57f0bc3d9edb
(Assignee)

Comment 32

7 years ago
No ill affects seems to have happened this time, closing it :)
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Comment 33

7 years ago
Comment on attachment 439330 [details] [diff] [review]
Yet again updated to trunk

>diff --git a/widget/src/qt/nsWindow.cpp b/widget/src/qt/nsWindow.cpp
>--- a/widget/src/qt/nsWindow.cpp
>+++ b/widget/src/qt/nsWindow.cpp
>@@ -1241,20 +1242,23 @@ nsWindow::InitButtonEvent(nsMouseEvent &
> nsEventStatus
> nsWindow::OnButtonPressEvent(QGraphicsSceneMouseEvent *aEvent)
> {
>+    // The user has done something.
>+    UserActivity();
>+
...
>@@ -1288,20 +1292,23 @@ nsWindow::OnButtonPressEvent(QGraphicsSc
> nsEventStatus
> nsWindow::OnButtonReleaseEvent(QGraphicsSceneMouseEvent *aEvent)
> {
>+    // The user has done something.
>+    UserActivity();
>+
...
> nsEventStatus
> nsWindow::OnKeyPressEvent(QKeyEvent *aEvent)
> {
>     LOGFOCUS(("OnKeyPressEvent [%p]\n", (void *)this));
> 
>+    // The user has done something.
>+    UserActivity();
>+
...
> nsEventStatus
> nsWindow::OnKeyReleaseEvent(QKeyEvent *aEvent)
> {
>     LOGFOCUS(("OnKeyReleaseEvent [%p]\n", (void *)this));
> 
>+    // The user has done something.
>+    UserActivity();
>+
...
>@@ -2483,10 +2496,22 @@ nsWindow::SetIMEEnabled(PRUint32 aState)
> NS_IMETHODIMP
> 
>+void
>+nsWindow::UserActivity()
>+{
>+  if (!mIdleService) {
>+    mIdleService = do_GetService("@mozilla.org/widget/idleservice;1");
>+  }
>+
>+  if (mIdleService) {
>+    mIdleService->ResetIdleTimeOut();
>+  }
>+}
>+

Was there a reason to just include button/key press/release as UserActivity? Now if browser is scrolled with button constantly pressed, observers are notified of idle state, since idle time just keeps going on.

Comment 34

7 years ago
and could this idle service be implemented without polling? I have added already a bug 584660 about not using it for Maemo 6 platform.
(Assignee)

Comment 35

7 years ago
(In reply to comment #33)
> 
> Was there a reason to just include button/key press/release as UserActivity?
> Now if browser is scrolled with button constantly pressed, observers are
> notified of idle state, since idle time just keeps going on.

Not as far as I remember...


(In reply to comment #34)
> and could this idle service be implemented without polling? I have added
> already a bug 584660 about not using it for Maemo 6 platform.

It depends on the platform, in the platform specific code nsIdleService<PLATFORM> the return value of the PollIdleTime function determines if that platform needs to run in poll mode or not.

Some platforms might need to run in polling mode and others don't have to.

Comment 36

7 years ago
(In reply to comment #35)
> (In reply to comment #33)
> > 
> > Was there a reason to just include button/key press/release as UserActivity?
> > Now if browser is scrolled with button constantly pressed, observers are
> > notified of idle state, since idle time just keeps going on.
> 
> Not as far as I remember...
> 
Ok added bug 594662 for counting scrolling and dragging also as interaction.

Updated

7 years ago
Depends on: 602085

Updated

6 years ago
Depends on: 670551
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.