Closed Bug 715041 Opened 12 years ago Closed 12 years ago

Add API for content to be notified on "OS idle"

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: cjones, Assigned: bsurender)

References

(Depends on 3 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(21 obsolete files)

This would provide basically the same thing that our current idle service does, but for general web content to listen to.  I can't think of any reasons why this would need to be a privileged API.

Strawman API proposal

  idleTimerId = setIdleTimeout(fn, timeMs)
  clearIdleTimeout(idleTimeId)

We need this immediately in gaia to implement various power-saving features like turning off the screen when the use has been idle for X seconds.
> I can't think of any reasons why this would need to be a privileged API.

Well...if we exposed the global idle service to unprivileged content, then content can tell the difference between "you're not using this tab" and "you're not using the browser".

Maybe this is a good thing.  For example, gmail changes your status to "away" when you don't interact with gmail for a while.  But with this service, Gmail could notice when you haven't used your *browser* in a while.

But it makes me kind of uncomfortable to be exposing this.  For example, there's a simple phishing technique where you send me to https://bank.com/login.html and then, after I look at the URL bar, redirect me to data:text/html,(evil) which looks the same as login.html.

The reason this isn't a particularly effective attack is that the evil site can't tell when I've stopped looking at the URL bar and clicked into the login box.  But with a global idle service, it could, and the attack could be much more convincing.

> Strawman API proposal

Maybe it would be more in line with the other DOM APIs to do

  addIdleListener(fn, timeMs)
  removeIdleListener(fn)

?
(In reply to Justin Lebar [:jlebar] from comment #1)
> But it makes me kind of uncomfortable to be exposing this.  For example,
> there's a simple phishing technique where you send me to
> https://bank.com/login.html and then, after I look at the URL bar, redirect
> me to data:text/html,(evil) which looks the same as login.html.
> 
> The reason this isn't a particularly effective attack is that the evil site
> can't tell when I've stopped looking at the URL bar and clicked into the
> login box.  But with a global idle service, it could, and the attack could
> be much more convincing.
> 

I don't follow how an idle notification makes this attack stronger.  The idea is to redirect off an idle notification?

> > Strawman API proposal
> 
> Maybe it would be more in line with the other DOM APIs to do
> 
>   addIdleListener(fn, timeMs)
>   removeIdleListener(fn)
> 
> ?

Well, I was shooting for consistency with setTimeout/setInterval, obviously.  But I don't particularly care.

We should also specify that UAs can clamp the min interval to a vendor-chosen minimum.  I'm not sure what a reasonable minimum would be; 1 second?  If we do that, maybe we should deliver the idle time along with the notification.
> I don't follow how an idle notification makes this attack stronger.  The idea is to redirect off 
> an idle notification?

Yes.  If the user does the following

 1. Click a link to load https://bank.com/login.html
 2. Wait for the page to load.
 3. Look at the URL.
 4. Click into the username field.

then she's idle during steps 2 and 3 and leaves idle as soon as she enters step 4.
Jesse, do you have any thoughts on the security implications of this API?
Is this intended to be "OS idle" or "Browser idle"?

I'm not worried about security:

* This API doesn't add much to unexpected-navigation attacks. They can be plenty effective without the API: switcharoo <http://lcamtuf.blogspot.com/2011/12/old-switcharoo.html>, navigating a background tab, or tabnapping <http://www.azarask.in/blog/post/a-new-type-of-phishing-attack/>.

* It also doesn't add much to "am I still in full-screen mode?" attacks, since a full-screen page can by definition tell whether the computer is in use.

But I am worried about privacy, and in particular attacks that correlate an anonymous identity with another identity. If both identities are constantly broadcasting their idleness, it becomes much easier to correlate them. Kind of like what Aaron Barr tried to do to members of Anonymous <http://arstechnica.com/tech-policy/news/2011/02/how-one-security-firm-tracked-anonymousand-paid-a-heavy-price.ars/2>.

A five minute clamp with a two minute fuzz would help. So a web page could tell that you've been idle for 5-7 minutes, but not exactly when you left.
(In reply to Justin Lebar [:jlebar] from comment #3)
> > I don't follow how an idle notification makes this attack stronger.  The idea is to redirect off 
> > an idle notification?
> 
> Yes.  If the user does the following
> 
>  1. Click a link to load https://bank.com/login.html
>  2. Wait for the page to load.
>  3. Look at the URL.
>  4. Click into the username field.
> 
> then she's idle during steps 2 and 3 and leaves idle as soon as she enters
> step 4.

But the attack still has to guess a specific idle-timeout, which doesn't seem more effective to me than guessing a setTimeout interval.  It would be more effective in hiding a failed attack attempt, though, since the attacker could bail on the redirection if the user doesn't go idle.
(In reply to Jesse Ruderman from comment #5)
> Is this intended to be "OS idle" or "Browser idle"?
> 

They're basically the same for the main user of this API, b2g.  But "browser idle" is more accurate.

> But I am worried about privacy, and in particular attacks that correlate an
> anonymous identity with another identity. If both identities are constantly
> broadcasting their idleness, it becomes much easier to correlate them. Kind
> of like what Aaron Barr tried to do to members of Anonymous
> <http://arstechnica.com/tech-policy/news/2011/02/how-one-security-firm-
> tracked-anonymousand-paid-a-heavy-price.ars/2>.
> 

Interesting.  Bug 714358 would be vulnerable to this as well.  I guess we should make both of these privileged.

> A five minute clamp with a two minute fuzz would help. So a web page could
> tell that you've been idle for 5-7 minutes, but not exactly when you left.

For b2g, we need idle notifications on the order of seconds to dim or turn off the screen.  But looks like this API needs privilege.
If we support this there definitely needs to be a way for TorButton to disable it (always return "busy"?). I suspect there are lots of users who will want a pref so they can do the same: there are similar prefs in various IM clients about whether your contacts can see if you're idle and I assume its existence in multiple clients is due to user demand.
This API will require a permission bit, but we need a kill switch anyway so disabling with a pref should be no sweat.

I think the IM client use case is different, because the responsibility is on the IM app to not show idle status.  Using a system-global pref for that means that yes, the IM app never sees an idle state, but also the screen manager can never dim and turn off the screen when the user is idle.  IOW, I don't want a setting I twiddle in an IM client to affect my screensaver.  And that's not how any IM client I've used works.
Assignee: nobody → anygregor
Here's what I propose security-wise:

For "plain" websites we expose the API but we add some amount of "fuzz" as to make correlating identities harder.

For installed apps we remove the fuzz.
> A five minute clamp with a two minute fuzz would help.

Just to clarify the clamp direction: We'd respect a request for "notify me after idle for X min" for any X greater than 5?

Not to be totally paranoid, but does the randomness for the fuzz need to be secure?  I presume JS could predict our RNG if it tried hard enough...
I'm not sure an idle timer clamped to >= 5 min would be all that useful.  But maybe, I guess.  Works for the IM case.

> Not to be totally paranoid, but does the randomness for the fuzz need to be
> secure?  I presume JS could predict our RNG if it tried hard enough...

I don't think it needs to be cryptographically secure.  Even if we used a linear congruential PRNG, I don't think it would be at all be easy for JS to predict the numbers if we seeded it properly.  Why do you think that?
> Even if we used a linear congruential PRNG, I don't think it would be at all be easy for JS to 
> predict the numbers if we seeded it properly.  Why do you think that?

It all depends on how paranoid you want to be, I guess.

The goal is to prevent an attacker from guessing the next random number, right?  So that means the seed needs to be secure, and, if we're not using a cryptographic RNG, the attacker can't have access to more than a few (more than just one?) random values.  Getting an approximate seed or an approximate random value is almost as bad as getting the exact seed or value.

If the PRNG was used only for idle fuzz and the seed was unpredictable, then an attacker could still read some values out of the PRNG by setting an idle timer a page he controls that the user might interact with.  The attacker could detect idleness using mousemove or whatever and using the idle service, and thus determine the fuzz.  This only needs to be successful a few times to blow the whole non-cryptographic PRNG.

If the PRNG is used for something other than idle fuzz (this was what I was thinking of in comment 12), then JS might be able to access the result of that PRNG and infer the likely next fuzz value.

Either way, seeding is still an issue.

I'm not saying we need to be this paranoid; there are lots of easier attacks on browsers.  But perhaps this attack is particularly bad.
I was assuming we would use a cryptographically secure random seed, in a PRNG (not crytographically secure) that content JS can't access.  I'm happy to kick the threat analysis to the secteam.
Assignee: anygregor → bsurender
Attached patch Function mozUserIdle patch (WIP) (obsolete) — Splinter Review
Moz User Idle patch.
Attachment #596107 - Flags: review?(bent.mozilla)
Comment on attachment 596107 [details] [diff] [review]
Function mozUserIdle patch (WIP)

To be clear, this is WIP.
Attachment #596107 - Attachment description: Function mozUserIdle patch → Function mozUserIdle patch (WIP)
Attachment #596107 - Flags: review?(bent.mozilla) → feedback?(bent.mozilla)
Associated bugs which interfere with idle/back notifications: 720493, 517482.
Attachment #596107 - Attachment is obsolete: true
Attachment #596107 - Flags: feedback?(bent.mozilla)
Attachment #599093 - Flags: review?(bent.mozilla)
Blocks: 712478
(In reply to Justin Lebar [:jlebar] from comment #1)

> But it makes me kind of uncomfortable to be exposing this.  For example,
> there's a simple phishing technique where you send me to
> https://bank.com/login.html and then, after I look at the URL bar, redirect
> me to data:text/html,(evil) which looks the same as login.html.

Hello,

Who could redirect me to data:text/html ? After I am at https://bank.com/login.html , I am in the hands of my secure bank. Only her could redirect me elsewhere. Or are you thinking of a frame combination using the evil "target" attribute ? I am imagining an attack in this way... Yet one more reason for Web browsers not to let any window/tab interact with a different window/tab.

Can you please enlighten me ?

Thanks,

Nicolas
Attached patch WIP Idle API (obsolete) — Splinter Review
WIP

- Currently includes printfs that were used for debugging. Does make the code a little ugly. Will cleanup when committing it to the central repo.

- Mochi tests in progress.
Attachment #608479 - Attachment is obsolete: true
- Adding and removing idle observers as per the new Idle API is complete.
- Includes printfs that were used to test real-time response sensitive functions. These will be removed once the mochi tests which is WIP are done.
- Idle API
- Completed
- Mochi tests completed
- Includes a lot of printfs which will be converted to nspr logging after the first review.
Attachment #599093 - Attachment is obsolete: true
Attachment #608486 - Attachment is obsolete: true
Attachment #599093 - Flags: review?(bent.mozilla)
Attachment #616366 - Flags: review?
- Idle API completed for first review
- Includes a lot of printfs which will be converted to nspr logging.
- Include mochi tests
Attachment #616366 - Attachment is obsolete: true
Attachment #616366 - Flags: review?
Attachment #616368 - Flags: review?
You need to ask for review from someone in particular -- bent?  Otherwise, your r? will just be ignored.
Attachment #616368 - Attachment is obsolete: true
Attachment #616368 - Flags: review?
Attachment #616371 - Flags: review?(bent.mozilla)
entered the requestee's email incorrectly which i didnt know until after the hitting the submit button at which point i got an error message along the lines of "requestee not found, so emailed the review to everyone"!
Attachment #616371 - Attachment is obsolete: true
Attachment #616371 - Flags: review?(bent.mozilla)
Attachment #617646 - Flags: review?(bent.mozilla)
Attachment #617646 - Attachment is obsolete: true
Attachment #617646 - Flags: review?(bent.mozilla)
Attachment #617653 - Flags: review?(bent.mozilla)
Try run for ff4190e5235e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ff4190e5235e
Results (out of 135 total builds):
    success: 26
    warnings: 101
    failure: 8
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bturner@mozilla.com-ff4190e5235e
Comment on attachment 617653 [details] [diff] [review]
Idle API completed for first try break.

Review of attachment 617653 [details] [diff] [review]:
-----------------------------------------------------------------

This is your first patch so I'm trying to be extra meticulous in order to show you how these reviews normally go, so don't be discouraged! We will get this fixed up in no time.

Overall this looks pretty good! A bunch of stuff to fix below, some minor, some not.

I think we need to do some architectural work here, too. We should start by removing the timer closure structs (see below), and then fix things to only ever have one timer (not two). We should also simplify the Add/Remove functions, we can chat about strategies later.

::: content/base/public/nsContentUtils.h
@@ +1826,5 @@
>  
>    /**
> +   * Returns true if the idle observers API is enabled.
> +   */
> +  static bool IsIdleObserversAPIEnabled();

Nit: I'd suggest "IsIdleObserverAPIEnabled" (no 's').

@@ +2016,5 @@
> +  /**
> +   * Function checks if the user is idle with respect to the window. 
> +   * Used in conjunction with GetMozUserIdle and mozUserIdle features.
> +   */
> +  static nsresult IsUserIdle(nsGlobalWindowObserver* mIdleObserver, bool* userIsIdle, PRUint32 requestedIdleTime);

Nit: We try to keep most code lines under 80 characters in length.

However, it looks like you don't actually use the first parameter, so please remove it.

@@ +2108,5 @@
>    static bool sAllowXULXBL_for_file;
>    static bool sIsFullScreenApiEnabled;
>    static bool sTrustedFullScreenOnly;
>    static bool sFullScreenKeyInputRestricted;
> +  static bool sIsIdleObserversAPIEnabled;

Nit: "sIsIdleObserverAPIEnabled"

::: content/base/src/nsContentUtils.cpp
@@ +907,5 @@
>    aType.StripWhitespace();
>  }
>  
> +nsresult 
> +nsContentUtils::IsUserIdle(nsGlobalWindowObserver* mIdleObserver, bool* userIsIdle, PRUint32 requestedIdleTime)

Nit: In general we try to name arguments with an "a" prefix ("a" == "argument"), so "aIdleObserver", "aUserIsIdle", etc. The "m" prefix is for "member", like class member.

Also, please make sure this fits under 80 chars.

@@ +909,5 @@
>  
> +nsresult 
> +nsContentUtils::IsUserIdle(nsGlobalWindowObserver* mIdleObserver, bool* userIsIdle, PRUint32 requestedIdleTime)
> +{
> +  *userIsIdle = false;

Nit: While harmless, I think it's much nicer to only set "out" arguments before returning a success code. If you return an error code then our convention is to ignore any "out" arguments anyway.

@@ +911,5 @@
> +nsContentUtils::IsUserIdle(nsGlobalWindowObserver* mIdleObserver, bool* userIsIdle, PRUint32 requestedIdleTime)
> +{
> +  *userIsIdle = false;
> +  nsresult rv;
> +  nsCOMPtr<nsIIdleService> idleService = do_GetService("@mozilla.org/widget/idleservice;1", &rv);

I think we should cache this service value, much as we do with sConsoleService.

@@ +912,5 @@
> +{
> +  *userIsIdle = false;
> +  nsresult rv;
> +  nsCOMPtr<nsIIdleService> idleService = do_GetService("@mozilla.org/widget/idleservice;1", &rv);
> +  if (NS_FAILED(rv)) {

Nit: Please use NS_ENSURE_SUCCESS here.

@@ +920,5 @@
> +  //Get the idle time in milli-seconds.
> +  PRUint32 idleTime;
> +  if (NS_FAILED(idleService->GetIdleTime(&idleTime))) {
> +    NS_WARNING("nsContentUtils::IsUserIdle() Failed to get current idle time.");
> +    return NS_ERROR_FAILURE;

This pattern can be simplified by using NS_ENSURE_SUCCESS:

  rv = idleService->GetIdleTime(&idleTime);
  NS_ENSURE_SUCCESS(rv, rv);

That will give you a warning on failure as well.

@@ +922,5 @@
> +  if (NS_FAILED(idleService->GetIdleTime(&idleTime))) {
> +    NS_WARNING("nsContentUtils::IsUserIdle() Failed to get current idle time.");
> +    return NS_ERROR_FAILURE;
> +  }
> +  else if (idleTime > requestedIdleTime) {

Nit: There's not much point in using "else" after a "return" statement, please remove.

@@ +926,5 @@
> +  else if (idleTime > requestedIdleTime) {
> +    *userIsIdle = true;
> +  }
> +  else {
> +    *userIsIdle = false;

Nit: This can be simplified:

  *userIsIdle = idleTime > requestedIdleTime;

However, you probably want ">=", not ">".

@@ +3685,5 @@
>    // make that safe and so we suppress it at this time. Ideally this should
>    // go away eventually.
>    NS_ASSERTION((aChild->IsNodeOfType(nsINode::eCONTENT) &&
>                 static_cast<nsIContent*>(aChild)->
> +               IsInNativeAnonymousSubtree()) ||

Not sure how this change snuck in here, but please revert it.

@@ +6488,5 @@
>  
> +bool
> +nsContentUtils::IsIdleObserversAPIEnabled()
> +{
> +  return sIsIdleObserversAPIEnabled;

Nit: I realize you're just following the previous pattern here, but this method can be inlined into the header.

::: content/events/public/nsEventNameList.h
@@ +517,5 @@
>                NS_MUTATION_EVENT)
>  NON_IDL_EVENT(DOMSubtreeModified,
>                NS_MUTATION_SUBTREEMODIFIED,
>                EventNameType_HTMLXUL,
>                NS_MUTATION_EVENT)

Nit: Please revert this change.

::: content/html/content/src/nsHTMLBodyElement.cpp
@@ +111,5 @@
>    NS_DECL_NSIDOMHTMLBODYELEMENT
>  
>    // Event listener stuff; we need to declare only the ones we need to
>    // forward to window that don't come from nsIDOMHTMLBodyElement.
> +

Nit: Please revert this change.

::: dom/base/Navigator.cpp
@@ +686,5 @@
>  
>  } // anonymous namespace
>  
>  NS_IMETHODIMP
> +Navigator::AddIdleObserver(nsIIdleObserver* idleObserver, JSContext* cx)

Hm, it doesn't appear that you ever use "cx", so let's not have a parameter of that type here.

Nit: Also, please prefix to "aIdleObserver"

@@ +688,5 @@
>  
>  NS_IMETHODIMP
> +Navigator::AddIdleObserver(nsIIdleObserver* idleObserver, JSContext* cx)
> +{
> +  NS_ENSURE_ARG_POINTER(idleObserver);

Nit: Newline after this.

@@ +689,5 @@
>  NS_IMETHODIMP
> +Navigator::AddIdleObserver(nsIIdleObserver* idleObserver, JSContext* cx)
> +{
> +  NS_ENSURE_ARG_POINTER(idleObserver);
> +  PRUint32 aIdleTimeInS;

Nit: "a" is the prefix we use for arguments. I'd suggest just "idleTimeInSec".

@@ +690,5 @@
> +Navigator::AddIdleObserver(nsIIdleObserver* idleObserver, JSContext* cx)
> +{
> +  NS_ENSURE_ARG_POINTER(idleObserver);
> +  PRUint32 aIdleTimeInS;
> +  idleObserver->GetTime(&aIdleTimeInS);

This call can fail (it's implemented in JS, and the JS could throw an exception). If the call fails then it will not change the value of "aIdleTimeInS". Since you've left that uninitialized you will be left with a garbage value. I suggest:

  nsresult rv = idleObserver->GetTime(...);
  if (NS_FAILED(rv)) {
    return rv;
  }

Ordinarily I like NS_ENSURE_SUCCESS for this, but it has a built in warning which I think is not useful here.

In general, I think warnings are very useful when our code fails - the messages can tip us off to something failing that really shouldn't. However, I don't think warnings are appropriate when a web page can trigger them.

@@ +693,5 @@
> +  PRUint32 aIdleTimeInS;
> +  idleObserver->GetTime(&aIdleTimeInS);
> +  // We don't accept idle time at 0, and we can't handle idle time that are too
> +  // high either - no more than ~136 years. Src. nsIdleService.cpp::AddIdleObserver()
> +  NS_ENSURE_ARG_RANGE(aIdleTimeInS, 1, (PR_UINT32_MAX / 10) - 1);

Hm, this range feels pretty arbitrary. Let's just say NS_ENSURE_ARG_MIN(idleTime, 1) and then let the idle service reject it if it doesn't like the time.

@@ +695,5 @@
> +  // We don't accept idle time at 0, and we can't handle idle time that are too
> +  // high either - no more than ~136 years. Src. nsIdleService.cpp::AddIdleObserver()
> +  NS_ENSURE_ARG_RANGE(aIdleTimeInS, 1, (PR_UINT32_MAX / 10) - 1);
> +
> +  if (nsContentUtils::IsIdleObserversAPIEnabled() && idleObserver != nsnull) {

Hm. Two things here.

First, you should check IsIdleObserverAPIEnabled before you do much else in this function. After all, if you're disabled then there's no reason to call GetIdleTime or do range checks.

Second, you've already NS_ENRSURE'd that idleObserver is non-null above, so this check is redundant.

@@ +697,5 @@
> +  NS_ENSURE_ARG_RANGE(aIdleTimeInS, 1, (PR_UINT32_MAX / 10) - 1);
> +
> +  if (nsContentUtils::IsIdleObserversAPIEnabled() && idleObserver != nsnull) {
> +    nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow);
> +    NS_ENSURE_TRUE(win, NS_OK);

This isn't ok, you want to return an error if somehow the window goes away underneath you.

@@ +698,5 @@
> +
> +  if (nsContentUtils::IsIdleObserversAPIEnabled() && idleObserver != nsnull) {
> +    nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow);
> +    NS_ENSURE_TRUE(win, NS_OK);
> +    win->AddIdleObserverToList(idleObserver);

This can fail so you need to check, and then NS_ENSURE_SUCCESS.

@@ +709,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +Navigator::RemoveIdleObserver(nsIIdleObserver* idleObserver, JSContext* cx)

Same unused cx argument, and please prefix the other arg.

@@ +713,5 @@
> +Navigator::RemoveIdleObserver(nsIIdleObserver* idleObserver, JSContext* cx)
> +{
> +  NS_ENSURE_ARG_POINTER(idleObserver);
> +  PRUint32 aIdleTimeInS;
> +  idleObserver->GetTime(&aIdleTimeInS);

As before, this can fail.

@@ +716,5 @@
> +  PRUint32 aIdleTimeInS;
> +  idleObserver->GetTime(&aIdleTimeInS);
> +  // We don't accept idle time at 0, and we can't handle idle time that are too
> +  // high either - no more than ~136 years. Src. nsIdleService.cpp::AddIdleObserver()
> +  NS_ENSURE_ARG_RANGE(aIdleTimeInS, 1, (PR_UINT32_MAX / 10) - 1);

Same comments as above (NS_ENSURE_ARG_MIN).

@@ +718,5 @@
> +  // We don't accept idle time at 0, and we can't handle idle time that are too
> +  // high either - no more than ~136 years. Src. nsIdleService.cpp::AddIdleObserver()
> +  NS_ENSURE_ARG_RANGE(aIdleTimeInS, 1, (PR_UINT32_MAX / 10) - 1);
> +
> +  if (nsContentUtils::IsIdleObserversAPIEnabled() && idleObserver != nsnull) {

Same comments as above (move higher).

@@ +720,5 @@
> +  NS_ENSURE_ARG_RANGE(aIdleTimeInS, 1, (PR_UINT32_MAX / 10) - 1);
> +
> +  if (nsContentUtils::IsIdleObserversAPIEnabled() && idleObserver != nsnull) {
> +    nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow);
> +    NS_ENSURE_TRUE(win, NS_OK);

Not ok.

@@ +721,5 @@
> +
> +  if (nsContentUtils::IsIdleObserversAPIEnabled() && idleObserver != nsnull) {
> +    nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow);
> +    NS_ENSURE_TRUE(win, NS_OK);
> +    win->RemoveIdleObserverFromList(idleObserver);

And this can fail.

::: dom/base/nsGlobalWindow.cpp
@@ +735,5 @@
>    {
>      if (!mWindow)
>        return NS_OK;
>      return mWindow->Observe(aSubject, aTopic, aData);
> +  } 

Nit: Looks like you added some trailing whitespace. Please revert.

@@ +740,3 @@
>    void Forget() { mWindow = nsnull; }
> +
> +  NS_IMETHODIMP GetInterface(const nsIID &iid, void **result) {

It looks like you can simplify this logic a little:

  if (iid.Equals(...) && mWindow) {
    return mWindow->QueryInterface(...);
  }
  return NS_NOINTERFACE;

@@ +870,5 @@
>  //*****************************************************************************
>  
>  nsGlobalWindow::nsGlobalWindow(nsGlobalWindow *aOuterWindow)
>    : nsPIDOMWindow(aOuterWindow),
> +    mCurrMinIdleTime(0),

Be careful to make sure that all of these new members get initialized, and that they get initialized in the correct order (same as in the declaration). Otherwise GCC will issue warnings.

@@ +947,5 @@
>          os->AddObserver(mObserver, "dom-storage2-changed", false);
>        }
> +      
> +      idleTimerCallbackS.idleWindow = this;
> +      idleTimerCallbackS.idleTimer = nsnull;

nsCOMPtr is auto-initialized to null, so this is unnecessary.

@@ +951,5 @@
> +      idleTimerCallbackS.idleTimer = nsnull;
> +      
> +      nsAutoString name;
> +      name.AssignLiteral("");
> +      idleTimerCallbackS.name = name;

strings are initialized to the empty string, so this is unnecessary.

@@ +954,5 @@
> +      name.AssignLiteral("");
> +      idleTimerCallbackS.name = name;
> +      
> +      localIdleTimerCallbackS.idleWindow = this;
> +      localIdleTimerCallbackS.localIdleTimer = nsnull;

Unnecessary.

@@ +1236,5 @@
>        os->RemoveObserver(mObserver, "dom-storage2-changed");
>      }
>  
> +    /*
> +    nsCOMPtr<nsIIdleService> idleService = do_GetService("@mozilla.org/widget/idleservice;1");

Hm... Doesn't look like this should be commented out! You'll definitely want to make sure that your window has been unregistered when this Cleanup function runs.

@@ +2693,5 @@
>  
>    aVisitor.mParentTarget = GetParentTarget();
> +
> +  //Handle 'back' aka 'active' event.
> +  if (NS_IS_TRUSTED_EVENT(aVisitor.mEvent) && (msg != NS_MOZ_USER_IDLE) &&

The NS_MOZ_USER_IDLE check is unnecessary since you're only going to do this for mouse/key events anyway.

@@ +2700,5 @@
> +       NS_IS_DRAG_EVENT(aVisitor.mEvent) ||
> +       NS_IS_KEY_EVENT(aVisitor.mEvent)
> +       )) {
> +    bool addFuzz = false;  
> +    if (mArrayIdleObservers.Length() > 0) {

For nsTArray, always use IsEmpty() if you're just testing that.

@@ +2701,5 @@
> +       NS_IS_KEY_EVENT(aVisitor.mEvent)
> +       )) {
> +    bool addFuzz = false;  
> +    if (mArrayIdleObservers.Length() > 0) {
> +      FireBackStatusEvent(addFuzz);

Just pass 'false' as the arg here, no need for the stack var.

@@ +8431,5 @@
>        nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(mDocShell));
>        if (webNav) {
>          *aSink = webNav;
>          NS_ADDREF(((nsISupports *) *aSink));
> +     }

Looks like you lost a space here.

@@ +8468,5 @@
>  
>    return *aSink ? NS_OK : NS_ERROR_NO_INTERFACE;
>  }
>  
> +class NotifyIdleObserverCaller : public nsRunnable {

Let's call this "NotifyIdleObserverRunnable"

@@ +8469,5 @@
>    return *aSink ? NS_OK : NS_ERROR_NO_INTERFACE;
>  }
>  
> +class NotifyIdleObserverCaller : public nsRunnable {
> + public:

Nit: This should be at col 0, no space.

@@ +8471,5 @@
>  
> +class NotifyIdleObserverCaller : public nsRunnable {
> + public:
> +  NotifyIdleObserverCaller(nsIIdleObserver *idleObserver, bool inCallOnidle) :
> +    mIdleObserver(idleObserver), callOnidle(inCallOnidle) {

Nit: here's the style we normally use:

  Foo(Arg1 aArg)
  : mMem1(aArg1)
  { }

@@ +8474,5 @@
> +  NotifyIdleObserverCaller(nsIIdleObserver *idleObserver, bool inCallOnidle) :
> +    mIdleObserver(idleObserver), callOnidle(inCallOnidle) {
> +  }
> +
> +  NS_IMETHOD Run() {

Nit: This can be simplified:

  return mCallOnIdle ? mIdleObserver->Onidle() : mIdleObserver->Onactive();

@@ +8487,5 @@
> + private:
> +  nsCOMPtr<nsIIdleObserver> mIdleObserver;
> +
> +  //if false then call on active
> +  bool callOnidle; 

Nit: remember, 'm' for members.

@@ +8496,5 @@
> +                                   bool callOnidle)
> +{
> +  nsCOMPtr<nsIRunnable> caller = 
> +    new NotifyIdleObserverCaller(idleObserverPtr, callOnidle);
> +  NS_DispatchToMainThread(caller);

This should always be on the main thread anyway, so please call NS_DispatchToCurrentThread (it's faster).

@@ +8502,5 @@
> +
> +nsresult
> +nsGlobalWindow::GetFuzzFactor(const PRUint32 randomMax, PRUint32* randNum)
> +{
> +  (*randNum) = 0;

No need to set this here, you only need to set if you're about to return a success code.

@@ +8505,5 @@
> +{
> +  (*randNum) = 0;
> + 
> +  PRSize bufSize = sizeof(randNum);
> +  PRSize nbytes = PR_GetRandomNoise(randNum, bufSize);

Hm, this is wrong and will corrupt memory. sizeof(randNum) will give you the size of a pointer, which can be different from the size of the actual integer you're going to set. You want:

  PR_GetRandomNoise(randNum, sizeof(*randNum));

@@ +8508,5 @@
> +  PRSize bufSize = sizeof(randNum);
> +  PRSize nbytes = PR_GetRandomNoise(randNum, bufSize);
> +  if (!nbytes) {
> +    fprintf(stderr, "Not implemented or no available noise!\n");
> +    return NS_ERROR_FAILURE;

We need to talk with jonas about this, not sure that a failure of random noise should prevent web pages from using the idle api entirely. But maybe we don't care. I'll find out :)

@@ +8511,5 @@
> +    fprintf(stderr, "Not implemented or no available noise!\n");
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  if ( (*randNum) < 0) {

This is an unsigned integer so this test will never return true. Please remove.

@@ +8515,5 @@
> +  if ( (*randNum) < 0) {
> +    (*randNum) *= -1;
> +  }
> +
> +  if ( (*randNum) > randomMax) {

Nit: Please remove extra parens

@@ +8516,5 @@
> +    (*randNum) *= -1;
> +  }
> +
> +  if ( (*randNum) > randomMax) {
> +    (*randNum) %= randomMax;

Hm, why are we doing the modulus here? Shouldn't we just assign randomMax?

@@ -8411,5 @@
>  void
>  nsGlobalWindow::FireOfflineStatusEvent()
>  {
> -  if (!mDoc)
> -    return;

Why did you remove this?

@@ +8533,5 @@
>      name.AssignLiteral("online");
>    }
> +  
> +  // The event is fired at the body element, 
> +  // or if there is no body element,

Please revert this change.

@@ +8555,5 @@
>    nsContentUtils::DispatchTrustedEvent(mDoc, eventTarget, name, true, false);
>  }
>  
>  nsresult
> +nsGlobalWindow::FireBackStatusEvent(const bool addFuzz)

Nit: the const is unnecessary here. Please remove.

@@ +8558,5 @@
>  nsresult
> +nsGlobalWindow::FireBackStatusEvent(const bool addFuzz)
> +{
> +  //Generate a random time between 0 and 120000 milliseconds
> +  const int randomMax = 120000; 

Nit: "Magic numbers" like this should be a #define near the top of the file, something like "MAX_IDLE_FUZZ_TIME_MS".

@@ +8563,5 @@
> +  bool fireEventNoFuzz = false;
> +
> +  //milli-seconds
> +  PRUint32 fuzzFactor = 0; 
> +  GetFuzzFactor(randomMax, &fuzzFactor);

You're not checking for failure here. Also, you don't need to call this if addFuzz is false, right?

@@ +8567,5 @@
> +  GetFuzzFactor(randomMax, &fuzzFactor);
> +
> +  nsresult rv;
> +  nsAutoString name;
> +  name.AssignLiteral(OBSERVER_TOPIC_BACK);

This can be replaced with:

  NS_NAMED_LITERAL_STRING(name, OBSERVER_TOPIC_BACK);

Although, I thought we were using "active", not "back"?

@@ +8569,5 @@
> +  nsresult rv;
> +  nsAutoString name;
> +  name.AssignLiteral(OBSERVER_TOPIC_BACK);
> +
> +  //Cancel all set idle timers. Re-using same idle timer.

Nit: There's only one timer, right? "all" implies more than one.

@@ +8572,5 @@
> +
> +  //Cancel all set idle timers. Re-using same idle timer.
> +  if (mIdleTimerSet) {
> +    if (mIdleTimer) {
> +      mIdleTimer->Cancel();

It seems impossible to have mIdleTimerSet be true, yet have no mIdleTimer, right? Please remove the mIdleTimer check and simply assert it.

@@ +8580,5 @@
> +  } 
> +
> +  // Do not add fuzz. I.e. immediate transition to active state.
> +  if (!addFuzz) {
> +

Nit: remove extra line

@@ +8581,5 @@
> +
> +  // Do not add fuzz. I.e. immediate transition to active state.
> +  if (!addFuzz) {
> +
> +    // Cancel all set 'back' timers if no fuzz requested.

Nit: "all" again.

@@ +8584,5 @@
> +
> +    // Cancel all set 'back' timers if no fuzz requested.
> +    if (mBackTimerSet) { 
> +      if (mIdleTimer) {
> +        mIdleTimer->Cancel();

Same comment here, replace check with assertion.

@@ +8600,5 @@
> +    }
> +  }
> +  else if (!mBackTimerSet) { 
> +    // if a back timer has NOT already been set then set one 
> +    // and fire a timer based back event.

Nit: Comment seems to suggest that you're going to also fire an event here. Maybe clarify to "If a back timer has not already been set then schedule one now."

@@ +8601,5 @@
> +  }
> +  else if (!mBackTimerSet) { 
> +    // if a back timer has NOT already been set then set one 
> +    // and fire a timer based back event.
> +    idleTimerCallbackS.idleWindow = this;

As far as I can tell this never changes. Why set this again?

@@ +8603,5 @@
> +    // if a back timer has NOT already been set then set one 
> +    // and fire a timer based back event.
> +    idleTimerCallbackS.idleWindow = this;
> +    if (!idleTimerCallbackS.idleTimer) { 
> +      mIdleTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);

Hm... It looks like you've always got "idleTimerCallbackS.idleTimer" set the same as mIdleTimer. Why do you need the additional member?

@@ +8605,5 @@
> +    idleTimerCallbackS.idleWindow = this;
> +    if (!idleTimerCallbackS.idleTimer) { 
> +      mIdleTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
> +      NS_ASSERTION(mIdleTimer, "nsGlobalWindow::nsGlobalWindow() OUT OF MEMORY "
> +                               "ERROR: Idle timer instance not created.");

You can't simply assert this since it is very possible that this will fail. You need to handle it.

@@ +8615,5 @@
> +    mIdleTimerSet = false;
> +    mBackTimerSet = true;
> +    printf("nsGlobalWindow::FireBackEvent() Fuzz Factor SET TO ZERO FOR "
> +           "TESTING PURPOSES. DISABLE BEFORE CHECKING IN.\n");
> +    fuzzFactor = 0;

We definitely don't want to check this in.

@@ +8622,5 @@
> +                                          fuzzFactor, 
> +                                          nsITimer::TYPE_ONE_SHOT);
> +    if (NS_FAILED(rv)) {
> +      NS_WARN_IF_FALSE(!NS_FAILED(rv), "nsGlobalWindow::FireBackStatusEvent() "
> +                                       "Failed to set back user timer callback!");

Er... You already know that rv failed, you just want NS_WARNING(...)

@@ +8626,5 @@
> +                                       "Failed to set back user timer callback!");
> +      mIdleTimerSet = false;
> +      mBackTimerSet = false;
> +    }
> +    printf("nsGlobalWindow::FireBackStatusEvent() BACK timer set\n");

This isn't true if the Init call failed... You probably want to return rv above if it does.

@@ +8629,5 @@
> +    }
> +    printf("nsGlobalWindow::FireBackStatusEvent() BACK timer set\n");
> +  }
> +  else {
> +    //do nothing.

This is obvious, no need for an empty else clause.

@@ +8638,5 @@
> +
> +void
> +nsGlobalWindow::FireIdleStatusEvent()
> +{
> +  nsAutoString name;

Doesn't look like you need this really. Just use NS_LITERAL_STRING below when you do.

@@ +8641,5 @@
> +{
> +  nsAutoString name;
> +
> +  //Generate a random time between 0 and 120000 milliseconds
> +  const int randomMax = 120000; 

Same thing here re: magic numbers

@@ +8645,5 @@
> +  const int randomMax = 120000; 
> +
> +  //fuzzFactor in milli-seconds
> +  PRUint32 fuzzFactor = 0; 
> +  GetFuzzFactor(randomMax, &fuzzFactor);

Same thing here re: checking for failure

@@ +8650,5 @@
> +
> +  bool userIsIdle = IsUserIdle(this);
> +  nsresult rv;
> +
> +  //Create idle timer instance.

Do you need to do this if userIsIdle is false?

@@ +8654,5 @@
> +  //Create idle timer instance.
> +  idleTimerCallbackS.idleWindow = this;
> +  if (!idleTimerCallbackS.idleTimer) { 
> +    mIdleTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
> +    NS_ASSERTION(mIdleTimer, "nsGlobalWindow::nsGlobalWindow() "

Same comment here re: handling failure.

@@ +8663,5 @@
> +
> +  if (userIsIdle) {  
> +    /* Cancel any back/active fuzz based timers that were set. 
> +     * mIdleTimer is used fuzz timing for both  idle and back/active events.
> +     * mIdleTimer can be used for fuzz timing ONLY one event (idle or back) 

Nit: this comment is pretty long, and redundant. I'd just keep the first sentence.

@@ +8668,5 @@
> +     * at a time!
> +     */
> +    if (mBackTimerSet) {
> +      if (mIdleTimer) {
> +        mIdleTimer->Cancel();

Assert rather than null check

@@ +8689,5 @@
> +      //for debugging only
> +      printf("nsGlobalWindow::FireIdleStatusEvent() Turned off fuzz factor "
> +             "here for debugging. Delete next line after debugging.\n");
> +      fuzzFactor = 0;
> +      //end debugging

All this needs to go.

@@ +8700,5 @@
> +        NS_WARNING("nsGlobalWindow::FireIdleStatusEvent() Failed to set "
> +                   "idle user timer callback!");
> +        mIdleTimerSet = false;
> +      }
> +      printf("nsGlobalWindow::FireIdleStatusEvent() idle timer set\n");

Again, this isn't always true.

@@ +8710,5 @@
> +void
> +nsGlobalWindow::IdleBackTimerCallback(nsITimer *aTimer, void *aClosure)
> +{
> +  IdleTimerCallbackStruct* idleTimerCallbackSPtr = 
> +                           (IdleTimerCallbackStruct *)aClosure;

static_cast please.

@@ +8715,5 @@
> +
> +  printf("\nnsGlobalWindow::IdleBackTimerCallback() Event: %s!\n", 
> +         NS_LossyConvertUTF16toASCII(idleTimerCallbackSPtr->name).get());
> +
> +  idleTimerCallbackSPtr->idleWindow->FireEvent(idleTimerCallbackSPtr->name);

Hm. It occurs to me that you already track which kind of event to fire with your two booleans, mIdleTimerSet and mBackTimerSet. Is there any reason you need to also track the name here?

Also, I don't think IdleTimerCallbackStruct is buying you anything here. You could just pass nsGlobalWindow* as your closure pointer and then access all the members you need directly.

@@ +8725,5 @@
> +  printf("nsGlobalWindow::ResetIdleServiceTimer() "
> +         "mArrayIdleObservers.Length() : "
> +         "%d\n", mArrayIdleObservers.Length());
> +
> +  if (mArrayIdleObservers.Length() > 0) {

!IsEmpty()

@@ +8727,5 @@
> +         "%d\n", mArrayIdleObservers.Length());
> +
> +  if (mArrayIdleObservers.Length() > 0) {
> +    if (mIdleTimer) {
> +      mIdleTimer->Cancel();

You can probably assert this too, right? (Seems impossible to have idleobservers in your array without also having a timer)

@@ +8738,5 @@
> +    nsresult rv = idleService->RemoveIdleObserver(mObserver, mCurrMinIdleTime);
> +    NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Failed to remove idle observer!");
> +    
> +    PRUint32 time;
> +    mArrayIdleObservers[0]->GetTime(&time);

This can fail, and will call JS... I think you should only ever call this GetTime function once (at 'AddIdleObserver' time) and then stash the value.

@@ +8744,5 @@
> +           "mCurrMinIdleTime %d timeS %d\n", time, mCurrMinIdleTime);
> +
> +    mCurrMinIdleTime = time;    
> +    rv = idleService->AddIdleObserver(mObserver, mCurrMinIdleTime);
> +    NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Failed to add idle observer!");  

This doesn't seem ok. It could fail, and then what would happen? You'd never get another notification. It seems like you need to communicate this to the caller somehow.

@@ +8764,5 @@
> +                         "Idle window not set in local idle "
> +                         "callback struct.");
> +
> +  PRUint32 userIdleTime;
> +  idleWindow->idleService->GetIdleTime(&userIdleTime);

Why not use the nsContentUtils function?

@@ +8784,5 @@
> +  
> +  MOZ_ASSERT(index < idleWindow->mArrayIdleObservers.Length(), 
> +             "nsGlobalWindow::LocalIdleTimerCallback() index > array length");
> +
> +  idleWindow->NotifyIdleObserver(idleWindow->mArrayIdleObservers[index], true);  

Let's use GetSafeElementAt() here. That will return null if we're out of bounds, and I'd feel safer that way.

@@ +8787,5 @@
> +
> +  idleWindow->NotifyIdleObserver(idleWindow->mArrayIdleObservers[index], true);  
> +
> +  PRUint32 currLocalTime;
> +  idleWindow->mArrayIdleObservers[index]->GetTime(&currLocalTime);

Again, this will run JS, and can fail. Please use a cached version.

@@ +8792,5 @@
> +  printf("nsGlobalWindow::LocalIdleTimerCallback() CURRENT idle "
> +         "observer time is: %d!\n", currLocalTime);
> +  
> +  //Reset local idle timer to the next idle observer.
> +  if ( (index+1) < idleWindow->mArrayIdleObservers.Length() ) {

Nit: Remove extra parens

@@ +8793,5 @@
> +         "observer time is: %d!\n", currLocalTime);
> +  
> +  //Reset local idle timer to the next idle observer.
> +  if ( (index+1) < idleWindow->mArrayIdleObservers.Length() ) {
> +    idleWindow->mLocalIdleTimerIndex = ++index;

So this bothers me a little... We now have "idleWindow->mLocalIdleTimerIndex", "index", and "localIdleTimerCallbackSPtr->localIdleTimerIndex", and all three are holding different copies of the same index that we care about. We should consolidate this stuff and only ever use "idleWindow->mLocalIdleTimerIndex", I think.

@@ +8795,5 @@
> +  //Reset local idle timer to the next idle observer.
> +  if ( (index+1) < idleWindow->mArrayIdleObservers.Length() ) {
> +    idleWindow->mLocalIdleTimerIndex = ++index;
> +    
> +    bool userIsIdle = idleWindow->IsUserIdle(idleWindow);

Use the nsContentUtils version

@@ +8796,5 @@
> +  if ( (index+1) < idleWindow->mArrayIdleObservers.Length() ) {
> +    idleWindow->mLocalIdleTimerIndex = ++index;
> +    
> +    bool userIsIdle = idleWindow->IsUserIdle(idleWindow);
> +    if (idleWindow->idleService != nsnull && userIsIdle) {

Why do you care about "idleWindow->idleService" here?

@@ +8798,5 @@
> +    
> +    bool userIsIdle = idleWindow->IsUserIdle(idleWindow);
> +    if (idleWindow->idleService != nsnull && userIsIdle) {
> +      PRUint32 nextLocalTime;
> +      idleWindow->mArrayIdleObservers[index]->GetTime(&nextLocalTime);

More JS again, and you're not checking error codes.

@@ +8803,5 @@
> +
> +      printf("nsGlobalWindow::LocalIdleTimerCallback() NEXT idle observer time "
> +             "is: %d!\n", nextLocalTime);
> +  
> +      PRUint32 callbackTime = (nextLocalTime - currLocalTime)*1000;

I think we need some safeguards here to make sure that a) this doesn't turn out to be negative, and b) we don't overflow.

@@ +8804,5 @@
> +      printf("nsGlobalWindow::LocalIdleTimerCallback() NEXT idle observer time "
> +             "is: %d!\n", nextLocalTime);
> +  
> +      PRUint32 callbackTime = (nextLocalTime - currLocalTime)*1000;
> +      NS_WARN_IF_FALSE(callbackTime >= 0, "nsGlobalWindow::LocalIdleTimerCallback() "

This is useless, you have an unsigned int here.

@@ +8816,5 @@
> +                 "idleWindow->mLocalIdleTimer is null!");
> +
> +      localIdleTimerCallbackSPtr->localIdleTimer = idleWindow->mLocalIdleTimer;
> +      localIdleTimerCallbackSPtr->localIdleTimerIndex = 
> +                                              idleWindow->mLocalIdleTimerIndex;

Yeah, these callback structs just hold members of the global window. No need for them, just use the global window as the closure.

@@ +8820,5 @@
> +                                              idleWindow->mLocalIdleTimerIndex;
> +      
> +      idleWindow->mMinLocalIdleTime = nextLocalTime;
> +      NS_WARN_IF_FALSE(nextLocalTime > 0, "nsGlobalWindow::LocalIdleTimerCallback() \
> +                                           mMinLocalIdleTime <= 0");

Another unsigned int, so < is impossible. You could assert != 0 though (not warn).

@@ +8828,5 @@
> +                                                          &(idleWindow->localIdleTimerCallbackS), 
> +                                                          callbackTime, 
> +                                                          nsITimer::TYPE_ONE_SHOT);
> +
> +      NS_WARN_IF_FALSE(!NS_FAILED(rv), "nsGlobalWindow::LocalIdleTimerCallback() "

You want this pattern:

  if (NS_FAILED(timer->Init(...))) {
    NS_WARNING("...");
  }

@@ +8834,5 @@
> +    }
> +    else {
> +      NS_WARN_IF_FALSE(false, "nsGlobalWindow::LocalIdleTimerCallback() No idle "
> +                              "service available! Failed to set local idle timer "
> +                              "callback!");

So, it looks to me like you can get here if userIsIdle is false. Isn't that completely possible?

@@ +8850,5 @@
> +    return;
> +  }
> +
> +  mLocalIdleTimer->Cancel();
> +  mLocalIdleTimer = nsnull;

Hm. Why null this out? Couldn't you reuse it?

@@ +8882,5 @@
> +  
> +  MOZ_ASSERT(mLocalIdleTimerIndex >= 0, "nsGlobalWindow::ResetLocalIdleTimer() "
> +                                        "mLocalIdleTimerIndex < 1");
> +
> +  PRInt32 mArrayIdleObserversLength = mArrayIdleObservers.Length();

PRUint32 here.

@@ +8890,5 @@
> +
> +  printf("nsGlobalWindow::ResetLocalIdleTimer() mLocalIdleTimerIndex: %d\n", 
> +         mLocalIdleTimerIndex);
> +
> +  rv = mArrayIdleObservers[mLocalIdleTimerIndex]->GetTime(&mMinLocalIdleTime);

Running JS again, and can fail.

@@ +8899,5 @@
> +  localIdleTimerCallbackS.localIdleTimer = mLocalIdleTimer;
> +  localIdleTimerCallbackS.localIdleTimerIndex = mLocalIdleTimerIndex;  
> +
> +  PRUint32 userIdleTimeMS; 
> +  idleService->GetIdleTime(&userIdleTimeMS);

This can fail, leaving you with an uninitialized garbage value.

@@ +8903,5 @@
> +  idleService->GetIdleTime(&userIdleTimeMS);
> +
> +  printf("nsGlobalWindow::ResetLocalIdleTimer() mLastLocalTimerTriggered %d\n", 
> +         mLastLocalTimerTriggered);
> +  if (!mLastLocalTimerTriggered) {

Hm... Do we need mLastLocalTimerTriggered? Can't you just check mLocalIdleTimerIndex == mArrayIdleObservers.Length() ?

@@ +8905,5 @@
> +  printf("nsGlobalWindow::ResetLocalIdleTimer() mLastLocalTimerTriggered %d\n", 
> +         mLastLocalTimerTriggered);
> +  if (!mLastLocalTimerTriggered) {
> +
> +    PRInt32 tmpCallbackTime = mMinLocalIdleTime*1000 - userIdleTimeMS;

Nit: spaces around the *

@@ +8906,5 @@
> +         mLastLocalTimerTriggered);
> +  if (!mLastLocalTimerTriggered) {
> +
> +    PRInt32 tmpCallbackTime = mMinLocalIdleTime*1000 - userIdleTimeMS;
> +    PRInt32 callbackTimeMin = -100;

Hm, this is a bit arbitrary.

@@ +8918,5 @@
> +      callbackTime = tmpCallbackTime;
> +    }
> +    else {
> +      MOZ_ASSERT(callbackTime > 0, "nsGlobalWindow::ResetLocalIdleTimer() "
> +                                   "callbackTime < 0.");

So, it looks entirely possible to end up here. All you'd need is for mMinLocalIdleTime to be 0, and then have userIdleTimeMS > 100. Then you'd end up here and you would use a uninitialized value for your timer delay.

@@ +8929,5 @@
> +                                               &localIdleTimerCallbackS, 
> +                                               callbackTime, 
> +                                               nsITimer::TYPE_ONE_SHOT);
> +
> +    NS_WARN_IF_FALSE(!NS_FAILED(rv), "nsGlobalWindow::ResetLocalIdleTimer() "

if (NS_FAILED(...)) { 
  NS_WARNING(...);
}

@@ +8945,5 @@
> +  
> +  PRUint32 userIdleTimeMS;
> +  idleService->GetIdleTime(&userIdleTimeMS);
> +  printf("nsGlobalWindow::FireEvent() userIdleTimeMS: %d\n", 
> +         userIdleTimeMS);

You don't actually use userIdleTimeMS except for logging, so make sure you put that in some kind of ifdef.

@@ +8950,5 @@
> +
> +
> +  if ( !strncmp(OBSERVER_TOPIC_IDLE, 
> +                (NS_LossyConvertUTF16toASCII(name).get()), 
> +                strlen(OBSERVER_TOPIC_IDLE) )) {

Here you can just use:

  name.EqualsLiteral(OBSERVER_TOPIC_IDLE)

However, what's the point of doing string comparisons here? All you want is a boolean for the argument to NotifyIdleObserver.

@@ +8957,5 @@
> +    //set local timer everytime just incase the currMinIdleTime has been
> +    //changed in nsGlobalWindow::AddIdleObserverToList()
> +    if (mArrayIdleObservers.Length() > 1 && mLocalIdleTimerIndex < 0) {
> +      mLocalIdleTimerIndex = 1;
> +      ResetLocalIdleTimer();

Hm, I don't understand what's going on here, and the comment doesn't make much sense...

@@ +8964,5 @@
> +  else if (!strncmp(OBSERVER_TOPIC_BACK, 
> +                    (NS_LossyConvertUTF16toASCII(name).get()), 
> +                    strlen(OBSERVER_TOPIC_BACK)) ) {
> +
> +    bool onIdleNotification = false;

Nit: No need for this stack var

@@ +8966,5 @@
> +                    strlen(OBSERVER_TOPIC_BACK)) ) {
> +
> +    bool onIdleNotification = false;
> +    CancelLocalIdleTimer();
> +    for (PRUint32 i=0; i<mArrayIdleObservers.Length(); i++) {

Nit: spaces around everything:

  for (PRUint32 i = 0; i < mArrayIdleObservers.Length(); i++)

@@ +8967,5 @@
> +
> +    bool onIdleNotification = false;
> +    CancelLocalIdleTimer();
> +    for (PRUint32 i=0; i<mArrayIdleObservers.Length(); i++) {
> +      NotifyIdleObserver(mArrayIdleObservers[i], onIdleNotification);

So, as I understand it, this will in essence queue up a runnable for each idle observer that will later call "onactive". So what happens if the user calls RemoveIdleObserver before those runnables run? Web pages will not expect those callbacks to run, so we need to fix this.

@@ +8970,5 @@
> +    for (PRUint32 i=0; i<mArrayIdleObservers.Length(); i++) {
> +      NotifyIdleObserver(mArrayIdleObservers[i], onIdleNotification);
> +    }
> +
> +    bool doCancelLocalIdleTimer = true;

Nit: No need for this stack var.

@@ +8982,5 @@
> +  printf("nsGlobalWindow::FireEvent() Done!\n");
> +}
> +
> +bool
> +nsGlobalWindow::IsUserIdle(nsGlobalWindow* globalWindow)

I think this function can be removed entirely.

@@ +8999,5 @@
> +  
> +  return false;
> +}
> +
> +//move from here to the top of the nsGlobalWindow.cpp later

In the future, these sorts of tasks should be completed before requesting review.

@@ +9008,5 @@
> +  
> +public:
> +  bool Equals(const A& a, const B& b) const {
> +    if (!a || !b) {
> +      return !a && !b;

Seems better to assert that a and b are always non-null.

@@ +9012,5 @@
> +      return !a && !b;
> +    }
> +    
> +    PRUint32 aTime = 0;
> +    nsresult rv = a->GetTime(&aTime);

Yeah, we really don't want to call this function again. Especially not in tight loops like this.

@@ +9024,5 @@
> +  }
> +  
> +  bool LessThan(const A& a, const B& b) const {
> +    if (!a || !b) {
> +      return !a && !b;

Assert again.

@@ +9028,5 @@
> +      return !a && !b;
> +    }
> +    
> +    PRUint32 aTime = 0;
> +    nsresult rv = a->GetTime(&aTime);

Can't do this :)

@@ +9043,5 @@
> +nsresult
> +nsGlobalWindow::AddIdleObserverToList(nsIIdleObserver* idleObserverPtr)
> +{
> +  nsresult rv;
> +  PRUint32 mArrayIdleObserversLength = mArrayIdleObservers.Length(); 

Nit: "m" for members only.

@@ +9046,5 @@
> +  nsresult rv;
> +  PRUint32 mArrayIdleObserversLength = mArrayIdleObservers.Length(); 
> + 
> +  if (!mArrayIdleObserversLength) {
> +    idleService = do_GetService("@mozilla.org/widget/idleservice;1");

If we use the nsContentUtils one then we shouldn't need this, right?

@@ +9054,5 @@
> +
> +  mIdleTimerSet = false;
> +  mBackTimerSet = false;
> +
> +  bool userIsIdle = IsUserIdle(this);   

I don't understand why you need to call this here. In just a second you get the idletime from the idle service, it seems easier to just compare the two numbers rather than calling the idle service again.

@@ +9055,5 @@
> +  mIdleTimerSet = false;
> +  mBackTimerSet = false;
> +
> +  bool userIsIdle = IsUserIdle(this);   
> +  PRUint32 timeS; //in seconds 

Hm, I think you should have a better name for this. There are lots of times floating around in this function.

@@ +9056,5 @@
> +  mBackTimerSet = false;
> +
> +  bool userIsIdle = IsUserIdle(this);   
> +  PRUint32 timeS; //in seconds 
> +  idleObserverPtr->GetTime(&timeS);

Here is the one and only time that we should be calling this function. Just need to handle exceptions and return if it fails.

@@ +9059,5 @@
> +  PRUint32 timeS; //in seconds 
> +  idleObserverPtr->GetTime(&timeS);
> +
> +  PRUint32 userIdleTimeMS; 
> +  idleService->GetIdleTime(&userIdleTimeMS); //returned in ms

This can fail, please check.

@@ +9082,5 @@
> +
> +  //add idle observer to empty list of idle observers
> +  mArrayIdleObservers.InsertElementSorted(idleObserverPtr, IdleObserverComparator());
> +  PRInt32 insertedElementIndex = mArrayIdleObservers.IndexOf(idleObserverPtr, 0, 
> +                                                             IdleObserverComparator());

So this will end up looping through the array twice, once to find the insertion point, then again to find the index. Instead you should unroll InsertElementSorted and call GreatestIndexLtEq and then call InsertElementAt.

@@ +9101,5 @@
> +           "setting mCurrMinIdleTime %d timeS %d\n", 
> +           timeS, mCurrMinIdleTime);
> +
> +    mCurrMinIdleTime = timeS;    
> +    userIsIdle = IsUserIdle(this); 

This is another call to the idle service when 1 should be sufficient.

@@ +9110,5 @@
> +    mLocalIdleTimer = nsnull;
> +    mLocalIdleTimerIndex = -1;
> +    
> +    if (mIdleTimer) {
> +      mIdleTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);

Please return rv if it fails.

@@ +9117,5 @@
> +                               "instance not created.");
> +    }
> +    
> +    rv = idleService->AddIdleObserver(mObserver, mCurrMinIdleTime);
> +    NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Failed to add idle observer!");

Please return any errors to the caller.

@@ +9124,5 @@
> +
> +  if (!userIsIdle) {
> +    if (timeS <= mCurrMinIdleTime) {
> +      rv = idleService->RemoveIdleObserver(mObserver, mCurrMinIdleTime);
> +      NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Failed to remove idle observer!");

Please return any errors to the caller.

@@ +9130,5 @@
> +      printf("nsGlobalWindow::AddIdleObserverToList() "
> +             "setting mCurrMinIdleTime %d timeS %d\n", time, mCurrMinIdleTime);
> +      
> +      mCurrMinIdleTime = timeS;
> +      rv = idleService->AddIdleObserver(mObserver, mCurrMinIdleTime);

Please return any errors to the caller.

@@ +9145,5 @@
> +         "mArrayIdleObservers.Length() %d\n", 
> +         mArrayIdleObservers.Length());
> +
> +  //user is idle from here onwards in the code
> +  PRUint32 userIdleTimeS = userIdleTimeMS/1000;

Nit: spaces here.

@@ +9146,5 @@
> +         mArrayIdleObservers.Length());
> +
> +  //user is idle from here onwards in the code
> +  PRUint32 userIdleTimeS = userIdleTimeMS/1000;
> +  if (timeS*1000 <= userIdleTimeMS) {

Nit: spaces here

@@ +9151,5 @@
> +    printf("nsGlobalWindow::AddIdleObserverToList() 1) "
> +           "userIsIdle && time < userIdleTimeS\n");
> +    NotifyIdleObserver(idleObserverPtr, true);
> +
> +    //add idle observer for 1s, add 3s, add 4s, after 3200ms add 2s.

Nit: I don't understand this comment.

@@ +9152,5 @@
> +           "userIsIdle && time < userIdleTimeS\n");
> +    NotifyIdleObserver(idleObserverPtr, true);
> +
> +    //add idle observer for 1s, add 3s, add 4s, after 3200ms add 2s.
> +    PRInt32 mArrayIdleObserversLength = mArrayIdleObservers.Length();

Hm, you already have a mArrayIdleObserversLength variable declared above. Maybe you don't need either? Above you could just do an IsEmpty() check before creating the timer, and here you could just test mArrayIdleObservers.Length() directly.

@@ +9153,5 @@
> +    NotifyIdleObserver(idleObserverPtr, true);
> +
> +    //add idle observer for 1s, add 3s, add 4s, after 3200ms add 2s.
> +    PRInt32 mArrayIdleObserversLength = mArrayIdleObservers.Length();
> +    if ( mLocalIdleTimerIndex != -1 && 

Nit: remove extra space here.

@@ +9159,5 @@
> +
> +      printf("nsGlobalWindow::AddIdleObserverToList() 2) "
> +             "mLocalIdleTimerIndex+1 < mArrayIdleObservers.Length()\n");
> +
> +      mLocalIdleTimerIndex++;      

This doesn't seem right. How do you know that you can do this?

@@ +9165,5 @@
> +    }
> +    printf("nsGlobalWindow::AddIdleObserverToList() 3\n");
> +    return NS_OK;
> +  }
> +  else if (mLocalIdleTimerIndex < 1) {

Nit: else after a return.

@@ +9168,5 @@
> +  }
> +  else if (mLocalIdleTimerIndex < 1) {
> +    printf("nsGlobalWindow::AddIdleObserverToList() 4\n");
> +
> +    //all existing idle observers in array where added with 

Nit: "were"

@@ +9171,5 @@
> +
> +    //all existing idle observers in array where added with 
> +    //requested notification time < user current idle time 
> +    //so no local timer was created.
> +    if (timeS == userIdleTimeS) {

I think you already handled this case above?

@@ +9183,5 @@
> +      printf("nsGlobalWindow::AddIdleObserverToList() 6\n");
> +      printf("nsGlobalWindow::AddIdleObserverToList() mLocalIdleTimerIndex < 1 "
> +             "&& userIsIdle && time > userIdleTimeS\n");
> +
> +      mLocalIdleTimerIndex = mArrayIdleObservers.Length() - 1;

Hm, why set this to the last? I really don't understand what's going on here. Seems to me that the next observer could have a time smaller than the currently requested time, but the local timer hasn't yet fired for that one. Something like:

  | 1 | 3 |

where 1 is the what the local timer is set for (index 0), and userIdleTimeS is 2. In this case you're going to blindly set the index to 1, skipping the first observer, right?

@@ +9187,5 @@
> +      mLocalIdleTimerIndex = mArrayIdleObservers.Length() - 1;
> +      mLastLocalTimerTriggered = false;
> +      ResetLocalIdleTimer();
> +    }
> +    printf("nsGlobalWindow::AddIdleObserverToList() 7\n");

Nit: You could return here, right?

@@ +9195,5 @@
> +    // but is < the current time that the local idle timer is set to
> +    // for a function callback therefore need to reset local idle timer
> +    // which already exists
> +    printf("nsGlobalWindow::AddIdleObserverToList() 8\n");
> +    mArrayIdleObservers[mLocalIdleTimerIndex]->GetTime(&mMinLocalIdleTime);

Running JS, can fail.

@@ +9198,5 @@
> +    printf("nsGlobalWindow::AddIdleObserverToList() 8\n");
> +    mArrayIdleObservers[mLocalIdleTimerIndex]->GetTime(&mMinLocalIdleTime);
> +    printf("nsGlobalWindow::AddIdleObserverToList() mLocalIdleTimerIndex %d, "
> +           "mMinLocalIdleTime: %d\n", mLocalIdleTimerIndex, mMinLocalIdleTime);
> +    mArrayIdleObserversLength = mArrayIdleObservers.Length();

Doesn't look like you use this.

@@ +9228,5 @@
> +nsresult
> +nsGlobalWindow::RemoveIdleObserverFromList(nsIIdleObserver* idleObserverPtr)
> +{
> +  PRUint32 time;
> +  idleObserverPtr->GetTime(&time);

This can fail, you need to check. Otherwise you use a garbage value.

@@ +9235,5 @@
> +  //check for removing from empty list.
> +  if (!mArrayIdleObservers.Length()) {
> +    NS_WARNING("nsGlobalWindow::RemoveIdleObserverFromList() trying to "
> +               "remove idle observer from empty list of idle observers!");
> +    return NS_ERROR_FAILURE;

In general we allow removing a non-existent observer to succeed (with a warning). NS_OK.

@@ +9242,5 @@
> +  //ensure that requested idle observer to be removed exists in the list.
> +  if ( !mArrayIdleObservers.Contains(idleObserverPtr, IdleObserverComparator()) ) {
> +    NS_WARNING("nsGlobalWindow::RemoveIdleObserverFromList() "
> +               "idle observer not found!");
> +    return NS_ERROR_FAILURE;

Same here.

@@ +9253,5 @@
> +                                IdleObserverComparator());
> +  mArrayIdleObservers.RemoveElementAt(removedElementIndex);
> +
> +  //no idle service warn.
> +  if (!idleService) {

Use nsContentUtils.

@@ +9261,5 @@
> +
> +  bool userIsIdle = IsUserIdle(this);
> +  
> +  //Removing only element in array of idle observers.
> +  if (!mArrayIdleObservers.Length()) {

IsEmpty()

@@ +9262,5 @@
> +  bool userIsIdle = IsUserIdle(this);
> +  
> +  //Removing only element in array of idle observers.
> +  if (!mArrayIdleObservers.Length()) {
> +    if (mIdleTimer) {

Should be able to assert this here.

@@ +9269,5 @@
> +    
> +    mLastLocalTimerTriggered = false;
> +    CancelLocalIdleTimer();
> +    nsresult rv = idleService->RemoveIdleObserver(mObserver, 
> +                                                  mCurrMinIdleTime);

Hm. mCurrMinIdleTime is changed a lot. Are you sure that this value is always going to be the same as the time when you called AddIdleObserver?

@@ +9283,5 @@
> +    return NS_OK;
> +  }
> +
> +  PRUint32 userIdleTimeMS; //returned in ms
> +  idleService->GetIdleTime(&userIdleTimeMS); 

Use nsContentUtils.

@@ +9295,5 @@
> +    }
> +
> +    bool doCancelLocalIdleTimer = true;
> +    if (!userIsIdle) {
> +      ResetIdleServiceTimer(doCancelLocalIdleTimer);

No need for stack var.

@@ +9351,4 @@
>  nsGlobalWindow::Observe(nsISupports* aSubject, const char* aTopic,
>                          const PRUnichar* aData)
>  {
> +  //for debugging

All this needs to be removed.

@@ +9380,5 @@
>  
> +  if (!nsCRT::strcmp(aTopic, OBSERVER_TOPIC_IDLE)) { 
> +    if (IsFrozen()) {
> +      // need to fire only one idle event while the window is frozen.
> +      mFireIdleStatusChangeEventOnThaw = !mFireIdleStatusChangeEventOnThaw;

Er, why not just "true"?

@@ +9395,5 @@
> +      mFireIdleStatusChangeEventOnThaw = false;
> +    }
> +    else {
> +      bool addFuzz = true;
> +      FireBackStatusEvent(addFuzz);

No need for stack var.

@@ +9396,5 @@
> +    }
> +    else {
> +      bool addFuzz = true;
> +      FireBackStatusEvent(addFuzz);
> +      

Nit: remove extra newline.

@@ +9517,5 @@
>        observer->Observe(aSubject, aTopic, aData);
>  
>      return NS_OK;
>    }
> +  

Nit: please revert.

@@ +9572,5 @@
>      mFireOfflineStatusChangeEventOnThaw = false;
>      FireOfflineStatusEvent();
>    }
>  
> +  if (mFireIdleStatusChangeEventOnThaw) {

How about the back event?

@@ +9923,5 @@
>      // create a timer and fire it off.
>  
>      timeout->mWhen = TimeStamp::Now() + delta;
>  
> +    timeout->mTimer = do_CreateInstance(NS_TIMER_CONTRACTID, &rv);//"@mozilla.org/timer;1", &rv);

Nit: Please revert.

@@ +11484,5 @@
>        do_GetService("@mozilla.org/globalmessagemanager;1");
>      mMessageManager =
>        new nsFrameMessageManager(true,
>                                  nsnull,
> +                               nsnull,

Nit: please revert.

@@ +11600,5 @@
>      if (!elm) {                                                              \
>        return NS_ERROR_OUT_OF_MEMORY;                                         \
>      }                                                                        \
>                                                                               \
> +    JSObject *obj = mJSObject;                                          \

Nit: please revert.

::: dom/base/nsGlobalWindow.h
@@ +267,5 @@
>  {
>  public:
>    friend class nsDOMMozURLProperty;
>  
> +  //The idle event timer object which is also re-used for back events fuzz timing.

Nit: Space between "//S" -> "// S". Please do this everywhere else here.

Also, this comment doesn't make much sense... You say that the timer "is also re-used" but never say what it is first used for.

And, this shouldn't be public (none of these members should).

@@ +268,5 @@
>  public:
>    friend class nsDOMMozURLProperty;
>  
> +  //The idle event timer object which is also re-used for back events fuzz timing.
> +  nsCOMPtr<nsITimer> mIdleTimer;

All of these member variables need to be moved lower to where the others are.

@@ +270,5 @@
>  
> +  //The idle event timer object which is also re-used for back events fuzz timing.
> +  nsCOMPtr<nsITimer> mIdleTimer;
> +
> +  /* Minimum idle time request of all the existing idle observers in SECONDS

Nit: Comment style in this file is to use //, not /**/. Need to fix this elsewhere.

Also, this comment could use some clarification.

@@ +276,5 @@
> +   * and nsIdleService RemoveIdleObserver code. 
> +   */
> +  PRUint32 mCurrMinIdleTime;
> +
> +  /* Minimum idle time request of all the existing idle observers in SECONDS

This comment seems incorrect.

@@ +283,5 @@
> +   */  
> +  nsCOMPtr<nsITimer> mLocalIdleTimer;
> +
> +  /* In SECONDS in compliance with the original nsIdleService AddIdleObserver 
> +   * and nsIdleService RemoveIdleObserver code. 

This one doesn't make sense either.

@@ +291,5 @@
> +
> +  //struct used for IdleTimerCallback function
> +  struct IdleTimerCallbackStruct {
> +    nsGlobalWindow* idleWindow;
> +    nsAutoString name;

nsString is what you want here.

@@ +303,5 @@
> +    PRInt32 localIdleTimerIndex;
> +  };
> +
> +  struct IdleTimerCallbackStruct idleTimerCallbackS;
> +  struct LocalIdleTimerCallbackStruct localIdleTimerCallbackS;

Nit: No need for the 'struct' here.

@@ +444,5 @@
>  
> +  virtual nsresult AddIdleObserverToList(nsIIdleObserver* idleObserverPtr);
> +  void CancelLocalIdleTimer();  
> +  void ResetLocalIdleTimer();
> +  virtual nsresult RemoveIdleObserverFromList(nsIIdleObserver* idleObserverPtr);

Add/Remove are both related, so let's group them together. Then, Cancel and Reset need comments saying what they do.

And, Cancel/Reset should not be public.

@@ +609,5 @@
>    void AddEventTargetObject(nsDOMEventTargetHelper* aObject);
>    void RemoveEventTargetObject(nsDOMEventTargetHelper* aObject);
>  
> +  //Notifies if an idle timer function callback has been set or not.
> +  bool mIdleTimerSet;

These need to be part of the bitset below, in the protected section.

@@ +627,5 @@
> +   *
> +   * @note: This function will cancel any timer based function callback
> +   *        set for idle notification.
> +   */
> +  nsresult FireBackStatusEvent(const bool addFuzz);

Hm, this one returns an nsresult, but FireIdleStatusEvent is void. Why different?

@@ +642,5 @@
> +   */
> +  void NotifyIdleObserver(nsIIdleObserver* idleObserverPtr, bool callOnidle);
> +  
> +  //Parameter randomMax must be in milliseconds!
> +  nsresult GetFuzzFactor(const PRUint32 randomMax, PRUint32* uRandNum);

Nit: This name needs to indicate that it has something to do with the idle events.

@@ +648,5 @@
>  protected:
> +  /**
> +   * Array/list of idle observers that are notified of idle events.
> +   */
> +  nsTArray< nsCOMPtr<nsIIdleObserver> > mArrayIdleObservers;

Nit: No need for the first space, nsTArray<nsCOMPtr<...> >.

@@ +649,5 @@
> +  /**
> +   * Array/list of idle observers that are notified of idle events.
> +   */
> +  nsTArray< nsCOMPtr<nsIIdleObserver> > mArrayIdleObservers;
> +  nsCOMPtr<nsIIdleService> idleService;

Nit: Move these below with the other member variables.

@@ +762,5 @@
>    // fire after it, but no earlier than mTimeoutInsertionPoint, if any.
>    void InsertTimeoutIntoList(nsTimeout *aTimeout);
>    static void TimerCallback(nsITimer *aTimer, void *aClosure);
> +  static void IdleBackTimerCallback(nsITimer *aTimer, void *closure);
> +  static void LocalIdleTimerCallback(nsITimer *aTimer, void *closure);

Need some comments about these... What do they do, and what's the difference between them?

@@ +783,5 @@
>                             const nsAString &aPopupWindowName,
>                             const nsAString &aPopupWindowFeatures);
>    void FireOfflineStatusEvent();
> +  void FireIdleStatusEvent();
> +  void ResetIdleServiceTimer(const bool doCancelLocalIdleTimer);

'const' is not important here, all args passed by value are copied anyway.

@@ +784,5 @@
>                             const nsAString &aPopupWindowFeatures);
>    void FireOfflineStatusEvent();
> +  void FireIdleStatusEvent();
> +  void ResetIdleServiceTimer(const bool doCancelLocalIdleTimer);
> +  void FireEvent(nsAutoString& name);

For passing strings as arguments always use 'const nsAString&' as the type (or 'const nsACString&' if you're using ASCII/UTF8). Otherwise the string object gets copied (not the contents, but hey, waste is waste).

Also, this function name is a little vague. What kinds of events does it fire? Etc.

@@ +785,5 @@
>    void FireOfflineStatusEvent();
> +  void FireIdleStatusEvent();
> +  void ResetIdleServiceTimer(const bool doCancelLocalIdleTimer);
> +  void FireEvent(nsAutoString& name);
> +  bool IsUserIdle(nsGlobalWindow* globalWindow);

It looks like you either want to make this static or lose the argument. But actually, can't you just remove it entirely and use the nsContentUtils version?

::: dom/base/nsPIDOMWindow.h
@@ +50,5 @@
>  #include "nsCOMPtr.h"
>  #include "nsEvent.h"
>  #include "nsIURI.h"
> +#include "jsapi.h"
> +#include "nsIDOMNavigator.h"

It doesn't look like any of these are needed.

@@ +103,5 @@
>                      "active state is only maintained on outer windows");
>      mIsActive = aActive;
>    }
>  
> +  virtual nsresult AddIdleObserverToList(nsIIdleObserver* idleObserverPtr)

Nit: After thinking about this a bit I think you should make these pure virtual. And let's use "RegisterIdleObserver" and "UnregisterIdleObserver".

::: dom/interfaces/base/nsIDOMNavigator.idl
@@ +117,5 @@
> +  /** 
> +   * Navigator requests to add an idle observer to the existing window.
> +   */
> +  [implicit_jscontext]
> +  void addIdleObserver(in nsIIdleObserver idleObserverPtr);

implicit_jscontext is no longer needed.

@@ +122,5 @@
> +  
> +  /** 
> +   * Navigator requests to remove an idle observer from the existing window.
> +   */
> +  [implicit_jscontext]

Here too.

@@ +159,5 @@
> +
> +[scriptable, uuid(e017de83-0aca-4683-99f1-926fbfb8a10b)]
> +interface nsIIdleObserver : nsISupports
> +{
> +  attribute unsigned long time;

Need to comment that this time is in seconds. And need to comment that it's only read at add/remove time, and subsequent changes have no effect.

::: dom/interfaces/base/nsIDOMWindow.idl
@@ +52,5 @@
>  interface nsIPrompt;
>  interface nsISelection;
>  interface nsIVariant;
>  
> +[scriptable, uuid(ce74975e-9592-4de3-897b-9825f4b6903f)]

Nit: Please revert all changes to this file.

::: modules/libpref/src/init/all.js
@@ +1306,5 @@
>  
>  // mouse wheel scroll transaction period of time (in milliseconds)
>  pref("mousewheel.transaction.timeout", 1500);
>  // mouse wheel scroll transaction is held even if the mouse cursor is moved.
> +pref("mousewheel.transaction.ignoremvedelay", 100);

Nit: please revert.

@@ +3520,5 @@
>  pref("full-screen-api.pointer-lock.enabled", true);
>  
> +// DOM idle observers API
> +pref("idle-observers-api.enabled", true);
> +

Nit: only one extra line here.

::: toolkit/content/tests/chrome/test_autocomplete3.xul
@@ +92,5 @@
>      start: 1, end: 1 },
>    { completeDefaultIndex: "true", key: "e", result: "result",
>      start: 2, end: 6 },
>    { completeDefaultIndex: "true", key: "t", result: "ret >> Result",
> +     start: 3, end: 13 }

Nit: please revert.

::: widget/gtk2/nsIdleServiceGTK.h
@@ -62,2 @@
>      nsIdleServiceGTK();
> -

Nit: Please revert.

::: widget/gtk2/nsWidgetFactory.cpp
@@ +140,5 @@
>  #endif
>  
>  #if defined(MOZ_X11)
> +NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR(nsIdleServiceGTK, 
> +                                         nsIdleServiceGTK::GetDerivedInstance<nsIdleServiceGTK>)

As discussed, this needs to happen for all the others too.

::: widget/nsGUIEvent.h
@@ +255,5 @@
>  #define NS_BEFORERESIZE_EVENT            (NS_WINDOW_START + 66)
>  
> +//Indicates that the user is either idle or active
> +#define NS_MOZ_USER_IDLE              (NS_WINDOW_START + 67)
> +#define NS_MOZ_USER_ACTIVE            (NS_WINDOW_START + 68)

Nit: please line these up with the others.

::: widget/xpwidgets/nsIdleService.cpp
@@ +83,5 @@
>  nsIdleServiceDaily::Observe(nsISupports *,
>                              const char *aTopic,
>                              const PRUnichar *)
>  {
> +  // Notify anyone who cares.

Nit: Please remove.

@@ +299,5 @@
>      mTimer->Cancel();
>    }
> +
> +  MOZ_ASSERT(gIdleService == this);
> +  gIdleService = NULL;

Nit: nsnull

@@ +304,3 @@
>  }
>  
> +NS_IMPL_ISUPPORTS3(nsIdleService, nsIdleService, nsIIdleService, nsIIdleServiceInternal)

the second entry here, "nsIdleService", should be removed. I hope that doesn't break something, we'll need to see what the tryserver says.
> Instead you should unroll InsertElementSorted and call GreatestIndexLtEq and then call
> InsertElementAt.

Or do this:

  PRInt32 insertedElementIndex =
    mArrayIdleObservers.InsertElementSorted(idleObserverPtr, IdleObserverComparator()) -
    mArrayIdleObservers.Elements();

Assuming this is an infallible array, of course.
Have you seen testing/mochitest/tests/SimpleTest/MockObjects.js?  It seems like that would simplify some of the XPCOM registration you're doing.
(In reply to Justin Lebar [:jlebar] from comment #35)
> Have you seen testing/mochitest/tests/SimpleTest/MockObjects.js?  It seems
> like that would simplify some of the XPCOM registration you're doing.

Will take a look at it. Thanks.
No longer blocks: b2g-demo-phone
Whiteboard: [b2g:blocking+]
blocking-basecamp: --- → +
blocking-kilimanjaro: --- → +
Whiteboard: [b2g:blocking+]
Attached patch WIP: Idle API review 2 (obsolete) — Splinter Review
WIP: Idle API review 2.
Attachment #617653 - Attachment is obsolete: true
Attachment #617653 - Flags: review?(bent.mozilla)
Attached patch Idle API review 2 (obsolete) — Splinter Review
Idle API review 2.

Ben as discussed yesterday, I have a beautiful green tree ... except for the Idle API mochi test across all platforms on the try server. I'm still debugging the Idle API mochi test failure on the loaner Linux64 test slave. The code as per our discussion on Thursday can be reviewed.

Thanks.
Attachment #629359 - Attachment is obsolete: true
Attachment #629363 - Flags: review?(bent.mozilla)
Idle API review 2.
Clean builds on try.
Passing all tests on try. Tree shows an orange due to existing test failure. Please refer to bug number 734680, https://bugzilla.mozilla.org/show_bug.cgi?id=734680
Attachment #629363 - Attachment is obsolete: true
Attachment #629363 - Flags: review?(bent.mozilla)
Attachment #630651 - Flags: review?(bent.mozilla)
Attached patch Idle API review 2. (obsolete) — Splinter Review
- Clean build, passing mochi test on try.
- Intermittent leak problem in oth test, bug number 734680, https://bugzilla.mozilla.org/show_bug.cgi?id=734680
- https://bugzilla.mozilla.org/show_bug.cgi?id=696306
Attachment #630651 - Attachment is obsolete: true
Attachment #630651 - Flags: review?(bent.mozilla)
Attachment #630778 - Flags: review?(bent.mozilla)
Attached patch Idle API review 2. (obsolete) — Splinter Review
Attached patch Idle API review 2. Passed try. (obsolete) — Splinter Review
Idle API review 2. Passed try.
Attachment #630778 - Attachment is obsolete: true
Attachment #631446 - Attachment is obsolete: true
Attachment #630778 - Flags: review?(bent.mozilla)
Attachment #631448 - Flags: review?(bent.mozilla)
Hey, question about https://wiki.mozilla.org/WebAPI/IdleAPI :

1) Is the IdleObserver being toss away as soon as the idle callback and active callback is called, or it gets to be called repetitively?
2) When does onactive actually being called on a B2G phone? Would it be called user presses a hardware button? Or when the user touches the screen that is in off state?

If these two question getting answered I can write the Gaia part first before this bug lands.

https://github.com/mozilla-b2g/gaia/issues/1532 is really annoying :(
Bent, this is waiting for review 6 days. Do you have an ETA for your review?
One more question:

3) can I pass null or undefined to either onidle or onactive if I only need one of them?
(In reply to Dietrich Ayala (:dietrich) from comment #44)
> Bent, this is waiting for review 6 days. Do you have an ETA for your review?

6 days is actually pretty good for me!

Hopefully Friday.
(In reply to ben turner [:bent] from comment #46)
> Hopefully Friday.

Er, hopefully I can start Friday.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #43)
> Hey, question about https://wiki.mozilla.org/WebAPI/IdleAPI :
> 
> 1) Is the IdleObserver being toss away as soon as the idle callback and
> active callback is called, or it gets to be called repetitively?
> 2) When does onactive actually being called on a B2G phone? Would it be
> called user presses a hardware button? Or when the user touches the screen
> that is in off state?
> 
> If these two question getting answered I can write the Gaia part first
> before this bug lands.
> 
> https://github.com/mozilla-b2g/gaia/issues/1532 is really annoying :(

https://wiki.mozilla.org/WebAPI/IdleAPI

1) An idle observer is registered with the Idle API and will be removed only when a call is made to removeIdleObserver(IdleObserver) as described in the link above.

The idle observer is repeatedly notified depending on the user state transition (idle/active) and only while the idle observer is registered with the idle API. I.e. if the user goes idle for the registered idle time then the idle observer will be notified in the registered amount of time that the user has gone idle. If the user is active then the idle observer is notified that the user is now active. The notifications are executed until the idle observer is unregistered from the Idle API.

2) The Idle Observer is notified i.e. on active is called when the user is active. This is when the user touches the screen when it is unlocked. Pressing a hardware button would turn the screen on but I'm not sure if the hardware button push would qualify as the user being active. I would have to confirm this. Unlocking the screen would trigger onactive but let me confirm this as well.

Hope this helps. Let me know if you have any questions. My IRC nic is bonnie on the webapi, developers and content channels.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #45)
> One more question:
> 
> 3) can I pass null or undefined to either onidle or onactive if I only need
> one of them?

3) This is a good question. Currently the idle API permits passing null for onidle and onactive. So yes you can pass null to either onidle or onactive or even pass null to both. This may not be a good thing as this permits developers to register idle observers with no onidle callback and no onactive callback, which could make registering with the idle service pointless. I will have to look into this and may restrict passing null to both but permit passing null any one of the two for an idle observer. :)
(In reply to bsurender from comment #48)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #43)
> > Hey, question about https://wiki.mozilla.org/WebAPI/IdleAPI :
> > 
> > 1) Is the IdleObserver being toss away as soon as the idle callback and
> > active callback is called, or it gets to be called repetitively?
> > 2) When does onactive actually being called on a B2G phone? Would it be
> > called user presses a hardware button? Or when the user touches the screen
> > that is in off state?
> > 
> > If these two question getting answered I can write the Gaia part first
> > before this bug lands.
> > 
> > https://github.com/mozilla-b2g/gaia/issues/1532 is really annoying :(
> 
> https://wiki.mozilla.org/WebAPI/IdleAPI
> 
> 1) An idle observer is registered with the Idle API and will be removed only
> when a call is made to removeIdleObserver(IdleObserver) as described in the
> link above.
> 
> The idle observer is repeatedly notified depending on the user state
> transition (idle/active) and only while the idle observer is registered with
> the idle API. I.e. if the user goes idle for the registered idle time then
> the idle observer will be notified in the registered amount of time that the
> user has gone idle. If the user is active then the idle observer is notified
> that the user is now active. The notifications are executed until the idle
> observer is unregistered from the Idle API.
> 
> 2) The Idle Observer is notified i.e. on active is called when the user is
> active. This is when the user touches the screen when it is unlocked.
> Pressing the power on/off hardware button would turn the screen on but I'm not sure if the
> this hardware button push would qualify as the user being active or actually unlocking the screen would qualify as the user being active. I would have to
> confirm this. From the code perspective events that qualify as mouse events or button push events on the screen qualify as active.
> 
> Hope this helps. Let me know if you have any questions. My IRC nic is bonnie
> on the webapi, developers and content channels.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #43)
> Hey, question about https://wiki.mozilla.org/WebAPI/IdleAPI :
> 
> 1) Is the IdleObserver being toss away as soon as the idle callback and
> active callback is called, or it gets to be called repetitively?
> 2) When does onactive actually being called on a B2G phone? Would it be
> called user presses a hardware button? Or when the user touches the screen
> that is in off state?
> 
> If these two question getting answered I can write the Gaia part first
> before this bug lands.
> 
> https://github.com/mozilla-b2g/gaia/issues/1532 is really annoying :(

2) onactive will be called when the user presses the power on/off button on the b2g phone which would turn the screen on.
https://github.com/mozilla-b2g/gaia/pull/1731

I've come up Gaia fix for this. You can try with it or point to me if there is anything wrong.
Comment on attachment 631448 [details] [diff] [review]
Idle API review 2. Passed try.

Review of attachment 631448 [details] [diff] [review]:
-----------------------------------------------------------------

This is better!

We still have some problems though. The main ones I see are:

1. You're calling into JS every time you want to know the idle time of an observer. As we discussed previously, you may only call into JS once for this information in AddIdleObserver and once in RemoveIdleObserver. We need to rework this so that you save everything away into a simple struct.

2. Your method for preventing a removed observer from firing after you've called NS_DispatchToCurrentThread is not precise enough and may fail. We'll need to fix that.

3. Your caching of the idle service on nsContentUtils is a little wonky and will leak.

4. I think we're missing some edge cases in the add/remove process. We should diagram to make sure.

5. Lots of cleanup needed. Most everything is noted below.

::: content/base/public/nsContentUtils.h
@@ +84,5 @@
>  class nsIImageLoadingContent;
>  class nsIDOMHTMLFormElement;
>  class nsIDOMDocument;
>  class nsIConsoleService;
> +class nsIIdleService;

Remove.

@@ +117,5 @@
>  struct nsIntMargin;
>  class nsPIDOMWindow;
>  class nsIDocumentLoaderFactory;
>  class nsIDOMHTMLInputElement;
> +class nsGlobalWindowObserver;

This looks unnecessary.

@@ +211,5 @@
>    typedef mozilla::dom::Element Element;
>    typedef mozilla::TimeDuration TimeDuration;
>  
>  public:
> +  static nsIIdleService* sIdleService;

We're not using this, please remove.

@@ +2020,5 @@
>  
> +  /**
> +   * Function checks if the user is idle.
> +   */
> +  static nsresult IsUserIdle(bool* aUserIsIdle, PRUint32 aRequestedIdleTime);

What unit of time are we supposed to pass here? Milliseconds, right? Please mention this in the comment and then rename that parameter to "aRequestedIdleTimeMS".

Nit: Our convention is to do out-params last. Please swap the order here.

@@ +2092,5 @@
>    static imgILoader* sImgLoader;
>    static imgICache* sImgCache;
>  
>    static nsIConsoleService* sConsoleService;
> +  

Nit: Please revert.

::: content/base/src/nsContentUtils.cpp
@@ +207,5 @@
>  #endif
>  imgILoader *nsContentUtils::sImgLoader;
>  imgICache *nsContentUtils::sImgCache;
>  nsIConsoleService *nsContentUtils::sConsoleService;
> +nsIIdleService *nsContentUtils::sIdleService;

Remove.

@@ +419,5 @@
>    Preferences::AddBoolVarCache(&sTrustedFullScreenOnly,
>                                 "full-screen-api.allow-trusted-requests-only");
>  
> +  Preferences::AddBoolVarCache(&sIsIdleObserverAPIEnabled,
> +                               "idle-observers-api.enabled");

We'll want to change this name, see below.

@@ +873,5 @@
>    aType.StripWhitespace();
>  }
>  
> +nsresult 
> +nsContentUtils::IsUserIdle(bool* aUserIsIdle, PRUint32 aRequestedIdleTime)

Please assert that aUserIsIdle is non-null.

@@ +880,5 @@
> +  nsCOMPtr<nsIIdleService> idleService = 
> +    do_GetService("@mozilla.org/widget/idleservice;1", &rv);
> +  
> +  if (NS_FAILED(rv)) {
> +    return rv;

NS_ENSURE_SUCCESS(rv, rv)

@@ +883,5 @@
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +  
> +  //Get the idle time in milli-seconds.

Nit: Space after //... Or you could just remove the comment. It doesn't tell you anything that reading the code doesn't already.

@@ +888,5 @@
> +  PRUint32 idleTime;
> +  if (NS_FAILED(idleService->GetIdleTime(&idleTime))) {
> +    NS_WARNING("nsContentUtils::IsUserIdle() "
> +               "Failed to get current idle time.");
> +    return NS_ERROR_FAILURE;

This is simpler:

  rv = idleService->GetIdleTime(&idleTime);
  NS_ENSURE_SUCCESS(rv, rv);

@@ +1425,5 @@
>      NS_IF_RELEASE(sStringBundles[i]);
>  
>    NS_IF_RELEASE(sStringBundleService);
>    NS_IF_RELEASE(sConsoleService);
> +  NS_IF_RELEASE(sIdleService);

Remove.

@@ +3802,5 @@
>    // make that safe and so we suppress it at this time. Ideally this should
>    // go away eventually.
>    NS_ASSERTION((aChild->IsNodeOfType(nsINode::eCONTENT) &&
>                 static_cast<nsIContent*>(aChild)->
> +                IsInNativeAnonymousSubtree()) ||

Nit: please revert

@@ +6636,5 @@
>    return sIsFullScreenApiEnabled;
>  }
>  
>  bool
> +nsContentUtils::IsIdleObserverAPIEnabled()

Please inline this in the header.

::: content/base/src/nsGkAtomList.h
@@ +666,5 @@
>  GK_ATOM(onpopstate, "onpopstate")
>  GK_ATOM(only, "only")               // this one is not an event
>  GK_ATOM(onmessage, "onmessage")
> +GK_ATOM(onmozuseridle, "onmozuseridle")
> +GK_ATOM(onmozuseractive, "onmozuseractive")

Revert.

::: content/events/public/nsEventNameList.h
@@ -498,5 @@
>  NON_IDL_EVENT(DOMSubtreeModified,
>                NS_MUTATION_SUBTREEMODIFIED,
>                EventNameType_HTMLXUL,
>                NS_MUTATION_EVENT)
> -

Please revert.

::: dom/base/Navigator.cpp
@@ +655,5 @@
> +Navigator::AddIdleObserver(nsIIdleObserver* aIdleObserver)
> +{
> +  if (!nsContentUtils::IsIdleObserverAPIEnabled()) {
> +    NS_WARNING("Navigator::AddIdleObserver() Either idle observer service "
> +               "preference has been turned off or null idle observer pointer.");

Nit: Please don't put the function name in the warning, the file and line number will be printed automatically.

Also, this warning is incorrect. IsIdleObserverAPIEnabled() only checks the preference. How about you just make it say "The IdleObserver API has been disabled".

Please fix the message here and below in RemoveIdleObserver.

@@ +662,5 @@
> +  
> +  NS_ENSURE_ARG_POINTER(aIdleObserver);
> +  
> +  PRUint32 idleTimeInS;
> +  nsresult rv = aIdleObserver->GetTime(&idleTimeInS);

This should be the only time you call into JS on the add path.

@@ +664,5 @@
> +  
> +  PRUint32 idleTimeInS;
> +  nsresult rv = aIdleObserver->GetTime(&idleTimeInS);
> +  if (NS_FAILED(rv)) {
> +    return rv;

NS_ENSURE_SUCCESS(rv, rv);

@@ +667,5 @@
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +  
> +  NS_ENSURE_ARG_MIN(idleTimeInS, 1);

Is this supposed to be MIN_IDLE_NOTIFICATION_TIME_S ?

@@ +670,5 @@
> +  
> +  NS_ENSURE_ARG_MIN(idleTimeInS, 1);
> +  
> +  nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);

This is not quite right, you want:

  nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow);
  NS_ENSURE_TRUE(win, NS_ERROR_UNEXPECTED);

@@ +671,5 @@
> +  NS_ENSURE_ARG_MIN(idleTimeInS, 1);
> +  
> +  nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  win->RegisterIdleObserver(aIdleObserver);

This can fail, you need to check.

@@ +682,5 @@
> +{
> +  if (!nsContentUtils::IsIdleObserverAPIEnabled()) {
> +    NS_WARNING("Navigator::RemoveIdleObserver() Either idle observer service "
> +               "preference has been turned off or null idle observer pointer.");
> +    return NS_OK;  

So this is interesting. You've set the preference stuff up as a variable cache, meaning that it will live-update if the user changes the pref while the browser is running. So what happens if the pref is enabled, someone adds an idle observer, and then flips the pref? If they then call remove it will silently fail, but you'll still deliver idle notifications. I think we need to talk about what should happen here.

@@ +688,5 @@
> +  
> +  NS_ENSURE_ARG_POINTER(aIdleObserver);
> +  
> +  PRUint32 idleTimeInS;
> +  nsresult rv = aIdleObserver->GetTime(&idleTimeInS);

Again, this should be the only time you call into JS on the removal path.

@@ +695,5 @@
> +  }
> +  
> +  NS_ENSURE_ARG_MIN(idleTimeInS, 1);
> +  
> +  nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow, &rv);

Same comment as above re: do_QueryReferent.

@@ +697,5 @@
> +  NS_ENSURE_ARG_MIN(idleTimeInS, 1);
> +  
> +  nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  win->UnregisterIdleObserver(aIdleObserver);

This can fail.

::: dom/base/nsGlobalWindow.cpp
@@ +495,5 @@
>   * An indirect observer object that means we don't have to implement nsIObserver
>   * on nsGlobalWindow, where any script could see it.
>   */
> +class nsGlobalWindowObserver : public nsIObserver, 
> +                               public nsIInterfaceRequestor {

Nit: Please put { on its own line.

@@ +508,2 @@
>      return mWindow->Observe(aSubject, aTopic, aData);
> +  } 

Nit: You added some trailing whitespace here.

@@ +511,3 @@
>    void Forget() { mWindow = nsnull; }
> +
> +  NS_IMETHODIMP GetInterface(const nsIID &iid, void **result) {

Nit: { on its own line. "aIID" and "aResult", and then put the & and ** on the left:

  NS_IMETHODIMP GetInterface(const nsIID& aIID, void** aResult)

@@ +714,5 @@
>          // events. Use a strong reference.
>          os->AddObserver(mObserver, "dom-storage2-changed", false);
>        }
> +      
> +      GetIdleBackFuzzFactor(&mIdleFuzzFactor);

This can fail, right? How would you handle that here in the constructor? I think you should move this to AddIdleObserver.

@@ +986,5 @@
>      if (os) {
>        os->RemoveObserver(mObserver, NS_IOSERVICE_OFFLINE_STATUS_TOPIC);
>        os->RemoveObserver(mObserver, "dom-storage2-changed");
>      }
> +    

Nit: Please revert.

@@ +1163,5 @@
>  DOMCI_DATA(Window, nsGlobalWindow)
>  
> +  // QueryInterface implementation for nsGlobalWindow
> +  NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsGlobalWindow)
> +

Nit: Please revert this whole block.

@@ +1268,5 @@
>  
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMARRAY(mPendingStorageEvents)
> +  for (PRUint32 i = 0; i < tmp->mArrayIdleObservers.Length(); i++) {
> +    NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mArrayIdleObservers[i]");
> +    cb.NoteXPCOMChild(tmp->mArrayIdleObservers.ElementAt(i));

NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY_OF_NSCOMPTR

@@ +1309,5 @@
>    NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mFocusedNode)
>  
>    NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMARRAY(mPendingStorageEvents)
>  
> +  tmp->mArrayIdleObservers.Clear();

NS_IMPL_CYCLE_COLLECTION_UNLINK_NSTARRAY

@@ +2467,5 @@
>    }
>  
>    aVisitor.mParentTarget = GetParentTarget();
> +
> +  //Handle 'back' aka 'active' event.

Nit: Space after //, and there's no "back" "event" any more.

@@ +2470,5 @@
> +
> +  //Handle 'back' aka 'active' event.
> +  if (NS_IS_TRUSTED_EVENT(aVisitor.mEvent) &&
> +      (NS_IS_MOUSE_EVENT(aVisitor.mEvent) || 
> +       NS_IS_MOUSE_LEFT_CLICK(aVisitor.mEvent) ||

This is already handled by NS_IS_MOUSE_EVENT

@@ +2473,5 @@
> +      (NS_IS_MOUSE_EVENT(aVisitor.mEvent) || 
> +       NS_IS_MOUSE_LEFT_CLICK(aVisitor.mEvent) ||
> +       NS_IS_DRAG_EVENT(aVisitor.mEvent) ||
> +       NS_IS_KEY_EVENT(aVisitor.mEvent)
> +       )) {

Nit: This should go on the previous line

@@ +2476,5 @@
> +       NS_IS_KEY_EVENT(aVisitor.mEvent)
> +       )) {
> +
> +    if (!mArrayIdleObservers.IsEmpty()) {
> +      mAddBackEventFuzz = false;

Why aren't you firing the active notification here? You're about to deliver another event to the page, it will know that it's active.

Also, how about you add this as your first condition above? E.g.:

  if (!mArrayIdleObservers.IsEmpty() &&
      NS_IS_TRUSTED_EVENT(aVisitor.mEvent) &&
      (NS_IS_MOUSE_EVENT(aVisitor.mEvent) || 
       ...)) {
    mAddBackEventFuzz = false;
  }

@@ -8311,5 @@
>    } else {
>      name.AssignLiteral("online");
>    }
> -  // The event is fired at the body element, or if there is no body element,
> -  // at the document.

Nit: please revert

@@ +8372,5 @@
>    }
>    nsContentUtils::DispatchTrustedEvent(mDoc, eventTarget, name, true, false);
>  }
>  
> +class NotifyIdleObserverRunnable : public nsRunnable {

Nit: { on its own line.

@@ +8374,5 @@
>  }
>  
> +class NotifyIdleObserverRunnable : public nsRunnable {
> +public:
> +  NotifyIdleObserverRunnable(nsIIdleObserver *aIdleObserver, bool aCallOnidle,

Nit: * on left

@@ +8380,5 @@
> +    : mIdleObserver(aIdleObserver), mIdleWindow(aIdleWindow),
> +      mCallOnidle(aCallOnidle)
> +  { }
> +
> +  NS_IMETHOD Run() {

Nit: { on its own line.

@@ +8381,5 @@
> +      mCallOnidle(aCallOnidle)
> +  { }
> +
> +  NS_IMETHOD Run() {
> +    if (mIdleWindow->mArrayIdleObservers.Contains(mIdleObserver)) {

This isn't sufficient. It's possible to add the same observer twice for different times, right? Then if you remove one you'll still run twice here. You need to find a more precise method here.

@@ +8402,5 @@
> +                                   nsGlobalWindow* aIdleWindow)
> +{
> +  nsCOMPtr<nsIRunnable> caller = 
> +    new NotifyIdleObserverRunnable(aIdleObserverPtr, aCallOnidle, aIdleWindow);
> +  NS_DispatchToCurrentThread(caller);

This can conceivably fail, though it really shouldn't. This is one of those places where you should do:

  if (NS_FAILED(...)) {
    NS_WARNING(...);
  }

@@ +8409,5 @@
> +// static
> +void
> +nsGlobalWindow::IdleBackTimerCallback(nsITimer *aTimer, void *aClosure)
> +{
> +  nsGlobalWindow* idleWindow = static_cast<nsGlobalWindow*>(aClosure);

You should assign this into a nsCOMPtr, and you should use dont_AddRef here to eat up your extra reference (that you're going to add below when you call InitTimer).

@@ +8411,5 @@
> +nsGlobalWindow::IdleBackTimerCallback(nsITimer *aTimer, void *aClosure)
> +{
> +  nsGlobalWindow* idleWindow = static_cast<nsGlobalWindow*>(aClosure);
> +  if (!idleWindow) {
> +    return;

This should never happen, right? Please assert it and lose the early return.

@@ +8415,5 @@
> +    return;
> +  }
> +  
> +  if (idleWindow->mIdleTopic) {
> +    idleWindow->FireIdleBackEvent(NS_LITERAL_STRING(OBSERVER_TOPIC_IDLE));

Rather than checking mIdleTopic here and then calling the method twice why not just do this check inside FireIdleBackEvent? Then you could remove the argument entirely.

@@ +8426,5 @@
> +// static
> +void
> +nsGlobalWindow::IdleObserverTimerCallback(nsITimer *aTimer, void *aClosure)
> +{
> +  nsGlobalWindow* idleWindow = static_cast<nsGlobalWindow*>(aClosure);

nsCOMPtr, dont_AddRef

@@ +8428,5 @@
> +nsGlobalWindow::IdleObserverTimerCallback(nsITimer *aTimer, void *aClosure)
> +{
> +  nsGlobalWindow* idleWindow = static_cast<nsGlobalWindow*>(aClosure);
> +  if (!idleWindow) {
> +    return;

Assert again.

@@ +8439,5 @@
> +    return;
> +  }
> +  idleWindow->NotifyIdleObserver(idleObserver, true, idleWindow);
> +  idleWindow->mIdleCallbackIndex++;
> +  nsresult rv = idleWindow->SetNextIdleObserverCallback();  

This is pointless... Are you intending to warn of that fails? If so:

  if (NS_FAILED(idleWindow->SetNextIdleObserverCallback()) {
  NS_WARNING(...);
}

@@ +8451,5 @@
> +    return NS_OK;
> +  }
> +
> +  nsIIdleObserver* idleObserver = 
> +    mArrayIdleObservers.SafeElementAt(mIdleCallbackIndex, nsnull);

You just did a bounds check above, no need for the "Safe" part here.

@@ +8454,5 @@
> +  nsIIdleObserver* idleObserver = 
> +    mArrayIdleObservers.SafeElementAt(mIdleCallbackIndex, nsnull);
> +
> +  PRUint32 requestedTimeMS = 0; 
> +  NS_ENSURE_SUCCESS(idleObserver->GetTime(&requestedTimeMS), NS_ERROR_FAILURE);

You can't call this again. Remember, we talked about this. You only want to call into JS once on Register and Unregister. Then you have to save this information so that you don't have to call GetTime again.

@@ +8467,5 @@
> +    callbackTimeMS = 0;
> +  }
> +  else {
> +    callbackTimeMS = requestedTimeMS - userIdleTimeMS;
> +    NS_ENSURE_SUCCESS(callbackTimeMS < PR_INT32_MAX, NS_ERROR_FAILURE);    

This is wrong. NS_ENSURE_SUCCESS checks for error codes. You want NS_ENSURE_TRUE.

Actually, why are you doing this at all? InitWithFuncCallback takes a PRUint32, and you're checking against PR_INT32_MAX (not PR_UINT32_MAX). What's going on here?

@@ +8471,5 @@
> +    NS_ENSURE_SUCCESS(callbackTimeMS < PR_INT32_MAX, NS_ERROR_FAILURE);    
> +  }
> +  
> +  nsresult rv = mIdleTimer->InitWithFuncCallback(IdleObserverTimerCallback, 
> +                                                 this, 

So, what ensures that 'this' window stays alive while the timer is running? You should addref here and then release in the callback.

@@ +8475,5 @@
> +                                                 this, 
> +                                                 callbackTimeMS, 
> +                                                 nsITimer::TYPE_ONE_SHOT);
> +  
> +  NS_ENSURE_SUCCESS(rv, rv);  

Nit: Let's move that newline below here, not have it above.

@@ +8483,5 @@
> +nsresult
> +nsGlobalWindow::GetIdleBackFuzzFactor(PRUint32* aRandNum)
> +{
> +  PRSize nbytes = PR_GetRandomNoise(aRandNum, sizeof(*aRandNum));
> +  if (!nbytes) {

I think you actually want

  if (nbytes != sizeof(*aRandNum))

@@ +8484,5 @@
> +nsGlobalWindow::GetIdleBackFuzzFactor(PRUint32* aRandNum)
> +{
> +  PRSize nbytes = PR_GetRandomNoise(aRandNum, sizeof(*aRandNum));
> +  if (!nbytes) {
> +    fprintf(stderr, "Not implemented or no available noise!\n");

NS_WARNING is all you need here.

@@ +8500,5 @@
> +nsGlobalWindow::FireBackStatusEvent()
> +{
> +  mIdleTopic = false;
> +  
> +  //milli-seconds

Nit: remove this comment

@@ +8503,5 @@
> +  
> +  //milli-seconds
> +  PRUint32 fuzzFactor = 0; 
> +  if (mAddBackEventFuzz) {
> +    NS_ENSURE_SUCCESS(GetIdleBackFuzzFactor(&fuzzFactor), NS_ERROR_FAILURE); 

nsresult rv = ...;
NS_ENSURE_SUCCESS(rv, rv);

@@ +8506,5 @@
> +  if (mAddBackEventFuzz) {
> +    NS_ENSURE_SUCCESS(GetIdleBackFuzzFactor(&fuzzFactor), NS_ERROR_FAILURE); 
> +  }
> +
> +  MOZ_ASSERT(mIdleTimer);

Nit: Move to top of this function

@@ +8509,5 @@
> +
> +  MOZ_ASSERT(mIdleTimer);
> +  mIdleTimer->Cancel();
> +
> +  // Do not add fuzz. I.e. immediate transition to active state.

Nit: This comment doesn't add anything useful here.

@@ +8528,5 @@
> +{
> +  mIdleTopic = true;
> +  MOZ_ASSERT(!mArrayIdleObservers.IsEmpty(), 
> +             "nsGlobalWindow::FireIdleStatusEvent() "
> +             "Trying to fire an idle event when there are 0 idle observers!");

Nit: Remove function name

@@ +8533,5 @@
> +
> +  bool userIsIdle = false; 
> +  NS_ENSURE_SUCCESS(nsContentUtils::IsUserIdle(&userIsIdle, 
> +                                               MIN_IDLE_NOTIFICATION_TIME_S), 
> +                    NS_ERROR_FAILURE);

nsresult rv = ...;
NS_ENSURE_SUCCESS(rv, rv);

@@ +8535,5 @@
> +  NS_ENSURE_SUCCESS(nsContentUtils::IsUserIdle(&userIsIdle, 
> +                                               MIN_IDLE_NOTIFICATION_TIME_S), 
> +                    NS_ERROR_FAILURE);
> +
> +  MOZ_ASSERT(mIdleTimer);

Nit: Move to top of function.

@@ +8551,5 @@
> +
> +nsresult
> +nsGlobalWindow::FireIdleBackEvent(const nsAString& aName)
> +{
> +  if (aName.EqualsLiteral(OBSERVER_TOPIC_IDLE)) {

You're having to do a string compare here when you could get away with just checking a boolean.

@@ +8558,5 @@
> +    return NS_OK;
> +  }     
> +
> +  if (aName.EqualsLiteral(OBSERVER_TOPIC_BACK)) {
> +

Nit: Get rid of extra newline.

@@ +8566,5 @@
> +      iter(mArrayIdleObservers);
> +    
> +    while (iter.HasMore()) {
> +      idleObserver = iter.GetNext();
> +      NS_ENSURE_ARG_POINTER(idleObserver);

This should never happen, right? Please assert it rather than early-returning.

@@ +8574,5 @@
> +    return NS_OK;
> +  }
> +  
> +  // Topic not "idle" or "back" passed as argument.
> +  return NS_ERROR_FAILURE;

You should also add NS_NOTREACHED("Unknown argument") here because it's a programmer error if you get here.

@@ +8581,5 @@
> +nsresult 
> +nsGlobalWindow::FindInsertIndexAt(nsIIdleObserver* aIdleObserver, 
> +                                  PRUint32* aInsertAtIndex) 
> +{
> +  *aInsertAtIndex = 0;

Nit: Please don't set this until right before you're about to return NS_OK (in all cases, of course).

@@ +8582,5 @@
> +nsGlobalWindow::FindInsertIndexAt(nsIIdleObserver* aIdleObserver, 
> +                                  PRUint32* aInsertAtIndex) 
> +{
> +  *aInsertAtIndex = 0;
> +  NS_ENSURE_ARG_POINTER(aIdleObserver);

This is an internal method, you should assert this and make sure you don't call it with null.

@@ +8588,5 @@
> +    return NS_OK;
> +  }
> +
> +  PRUint32 idleObserverRequestedTimeS;
> +  NS_ENSURE_SUCCESS(aIdleObserver->GetTime(&idleObserverRequestedTimeS), 

Don't call this, it will call into JS.

@@ +8592,5 @@
> +  NS_ENSURE_SUCCESS(aIdleObserver->GetTime(&idleObserverRequestedTimeS), 
> +                    NS_ERROR_FAILURE);
> +
> +  PRUint32 requestedTimeS = 0; 
> +  nsIIdleObserver* idleObserver;

This should be a nsCOMPtr. Otherwise the observer could die while you're calling it.

@@ +8598,5 @@
> +    iter(mArrayIdleObservers);
> +
> +  while (iter.HasMore()) {
> +    idleObserver = iter.GetNext();
> +    NS_ENSURE_SUCCESS(idleObserver->GetTime(&requestedTimeS), NS_ERROR_FAILURE);

Don't call this, it will call into JS.

@@ +8602,5 @@
> +    NS_ENSURE_SUCCESS(idleObserver->GetTime(&requestedTimeS), NS_ERROR_FAILURE);
> +    if (requestedTimeS > idleObserverRequestedTimeS) {
> +      break;
> +    }
> +    (*aInsertAtIndex)++;

Nit: I think it's much nicer to have a local stack variable that you increment and then set to your outparam later. But this is correct so up to you.

@@ +8603,5 @@
> +    if (requestedTimeS > idleObserverRequestedTimeS) {
> +      break;
> +    }
> +    (*aInsertAtIndex)++;
> +    NS_ENSURE_SUCCESS( *aInsertAtIndex <= mArrayIdleObservers.Length(), 

This should be asserted, not ensured.

@@ +8611,5 @@
> +}
> +
> +nsresult 
> +nsGlobalWindow::InsertIdleObserverAt(nsIIdleObserver* aIdleObserver, 
> +                                     PRUint32 aInsertAtIndex)

You should assert that the index <= length of your array here.

@@ +8616,5 @@
> +{
> +  if (aInsertAtIndex == mArrayIdleObservers.Length()) {
> +    nsCOMPtr<nsIIdleObserver>* insertedIdleObserver = 
> +      mArrayIdleObservers.AppendElement(aIdleObserver);
> +    NS_ENSURE_ARG_POINTER(insertedIdleObserver);

These arrays are infallible now, meaning that if it fails the entire program will abort. You don't need to null-check here, or below.

@@ +8630,5 @@
> +nsresult
> +nsGlobalWindow::RegisterIdleObserver(nsIIdleObserver* aIdleObserverPtr)
> +{    
> +  if (mArrayIdleObservers.IsEmpty()) {
> +    //if (!nsContentUtils::sIdleService) {

Nit: Commented code should almost never be checked in. If it's unimportant then it should most likely be removed. If it is important then it should probably be explained in a more verbose way.

@@ +8634,5 @@
> +    //if (!nsContentUtils::sIdleService) {
> +      nsresult rv = CallGetService("@mozilla.org/widget/idleservice;1", 
> +                                   &nsContentUtils::sIdleService);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      //}

We need to rework this. There's two big problems here:

1. CallGetService will AddRef the service each time it's called, and if you don't call Release the same number of times you'll leak. You're replacing a raw pointer here without keeping the refcount in mind.

2. You really shouldn't be touching nsContentUtils members. This means a) that sIdleService isn't private, like it should be, and b) you're not practicing good information-hiding. In general you should have methods on nsContentUtils that can retrieve the service for you and let nsContentUtils worry about where and how to cache it.

@@ +8638,5 @@
> +      //}
> +        
> +    NS_ENSURE_SUCCESS(nsContentUtils::sIdleService->
> +                      AddIdleObserver(mObserver, MIN_IDLE_NOTIFICATION_TIME_S), 
> +                      NS_ERROR_FAILURE);

rv = ...;
NS_ENSURE_SUCCESS(rv, rv);

@@ +8640,5 @@
> +    NS_ENSURE_SUCCESS(nsContentUtils::sIdleService->
> +                      AddIdleObserver(mObserver, MIN_IDLE_NOTIFICATION_TIME_S), 
> +                      NS_ERROR_FAILURE);
> +    if (!mIdleTimer) { 
> +      nsresult rv;

You already have an rv.

@@ +8641,5 @@
> +                      AddIdleObserver(mObserver, MIN_IDLE_NOTIFICATION_TIME_S), 
> +                      NS_ERROR_FAILURE);
> +    if (!mIdleTimer) { 
> +      nsresult rv;
> +      mIdleTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);     

NS_TIMER_CONTRACTID

@@ +8647,5 @@
> +    }
> +  }
> +
> +  PRUint32 insertAtIndex = 0;
> +  FindInsertIndexAt(aIdleObserverPtr, &insertAtIndex); 

This can fail, you should check.

@@ +8651,5 @@
> +  FindInsertIndexAt(aIdleObserverPtr, &insertAtIndex); 
> +
> +  PRUint32 idleObserverRequestedTimeS;
> +  NS_ENSURE_SUCCESS(aIdleObserverPtr->GetTime(&idleObserverRequestedTimeS), 
> +                    NS_ERROR_FAILURE);

rv = ...;
NS_ENSURE_SUCCESS(rv, rv);

@@ +8652,5 @@
> +
> +  PRUint32 idleObserverRequestedTimeS;
> +  NS_ENSURE_SUCCESS(aIdleObserverPtr->GetTime(&idleObserverRequestedTimeS), 
> +                    NS_ERROR_FAILURE);
> +  MOZ_ASSERT(mIdleTimer);

Nit: I'd move this up just after the mArrayIdleObservers.IsEmpty check. Otherwise it sort of implies that the GetTime call was somehow going to set this.

@@ +8656,5 @@
> +  MOZ_ASSERT(mIdleTimer);
> +  bool userIsIdle = false; 
> +  NS_ENSURE_SUCCESS(nsContentUtils::IsUserIdle(&userIsIdle, 
> +                                               MIN_IDLE_NOTIFICATION_TIME_S), 
> +                    NS_ERROR_FAILURE);

rv = ...;
NS_ENSURE_SUCCESS(rv, rv);

@@ +8658,5 @@
> +  NS_ENSURE_SUCCESS(nsContentUtils::IsUserIdle(&userIsIdle, 
> +                                               MIN_IDLE_NOTIFICATION_TIME_S), 
> +                    NS_ERROR_FAILURE);
> +
> +  

Nit: remove extra newline.

@@ +8660,5 @@
> +                    NS_ERROR_FAILURE);
> +
> +  
> +  if (mIdleCallbackIndex >= 0 && insertAtIndex < mIdleCallbackIndex) {
> +    mIdleTimer->Cancel();

Hm, why do you need to cancel the timer here?

@@ +8674,5 @@
> +    PRUint32 userIdleTimeMS;
> +    NS_ENSURE_SUCCESS(nsContentUtils::sIdleService->GetIdleTime(&userIdleTimeMS), 
> +                      NS_ERROR_FAILURE);  
> +
> +    mIdleTimer->Cancel();

Same here.

@@ +8686,5 @@
> +    }
> +  }  
> +  else {
> +    InsertIdleObserverAt(aIdleObserverPtr, insertAtIndex);
> +    // If the user is already idle & last idle observer already notified

This comment dosn't seem to match what is being tested in this block.

@@ +8688,5 @@
> +  else {
> +    InsertIdleObserverAt(aIdleObserverPtr, insertAtIndex);
> +    // If the user is already idle & last idle observer already notified
> +    PRUint32 userIdleTimeMS;
> +    NS_ENSURE_SUCCESS(nsContentUtils::sIdleService->GetIdleTime(&userIdleTimeMS), 

It's weird to use both sIdleService->GetIdleTime and nsContentUtils::IsUserIdle. They're both basically the same thing, right? We need to clean this up and maybe remove nsContentUtils::IsUserIdle if it's not precise enough on its own.

@@ +8694,5 @@
> +    if (idleObserverRequestedTimeS*1000 <= userIdleTimeMS) {
> +      if (mIdleCallbackIndex == -1) {
> +        mIdleCallbackIndex++;
> +      }
> +      NotifyIdleObserver(aIdleObserverPtr, true, this);

I don't think this is right... the requested idle time is less than the current idle time, why would you notify now?

@@ +8699,5 @@
> +      mIdleCallbackIndex++;
> +    }
> +    
> +    // inserted idle observer past previously last idle observer in array.
> +    // need to set callback to now new last element

I think we're missing some cases here. Let's do a diagram later.

@@ +8715,5 @@
> +nsresult
> +nsGlobalWindow::UnregisterIdleObserver(nsIIdleObserver* aIdleObserverPtr)
> +{  
> +  // Check for removing from empty list.
> +  NS_ENSURE_SUCCESS(!mArrayIdleObservers.IsEmpty(), NS_ERROR_FAILURE);

In general we return NS_OK for removing something that doesn't exist. Maybe a warning can be issued, but that's it. We shouldn't throw JS exceptions.

@@ +8727,5 @@
> +  NS_ENSURE_SUCCESS(nsContentUtils::IsUserIdle(&userIsIdle, 
> +                                               MIN_IDLE_NOTIFICATION_TIME_S), 
> +                    NS_ERROR_FAILURE);
> +
> +  //Remove element from array/list of idle observers

Nit: space after //

@@ +8742,5 @@
> +    return NS_OK;
> +  }
> +  
> +  if (!userIsIdle) {
> +    return NS_OK;

Hm... Why are we bailing out here? Seems to me that we should not check this here.

@@ +8746,5 @@
> +    return NS_OK;
> +  }
> +
> +  if (removeElementIndex < mIdleCallbackIndex) {
> +    mIdleTimer->Cancel();

Canceling should not be needed.

@@ -8462,5 @@
>        observer->Observe(aSubject, aTopic, aData);
>  
>      return NS_OK;
>    }
> -

Nit: Please revert.

::: dom/base/nsGlobalWindow.h
@@ +61,5 @@
>  #include "nsFrameMessageManager.h"
>  #include "mozilla/TimeStamp.h"
>  #include "nsIDOMTouchEvent.h"
>  #include "nsIInlineEventHandlers.h"
> +#include "nsIIdleService.h"

Please revert.

@@ +79,5 @@
>  // During click or mousedown events (and others, see nsDOMEvent) we allow modal
>  // dialogs up to this limit, even if they were disabled.
>  #define MAX_DIALOG_COUNT 10
>  
> +//Generate a random time between 0 and 120000 milliseconds

Nit: Space after //. And this comment doesn't make sense. What does this #define do? It doesn't "generate" anything, it simply sets a limit on how much randomness will be incorporated into the callbacks. Also, make sure to put a "." after it.

@@ +100,5 @@
>  class nsPerformance;
>  class nsIDocShellLoadInfo;
>  class WindowStateHolder;
>  class nsGlobalWindowObserver;
> +class IdleObserver;

What is this?

@@ +237,5 @@
>                         public nsIInlineEventHandlers
>  {
>  public:
> +  // Array/list of idle observers that are notified of idle events.
> +  nsTObserverArray<nsCOMPtr<nsIIdleObserver> > mArrayIdleObservers;

Nit: Please put this member with all the other members, not here in the typedefs and methods section.

@@ +555,5 @@
> +   *
> +   * @return: void
> +   */
> +  void NotifyIdleObserver(nsIIdleObserver* aIdleObserverPtr, bool aCallOnidle,
> +                          nsGlobalWindow* aIdleWindow);  

There is no reason to pass the window here, it's always "this".

Nit: So here you only mention two parameters when there are three. Also you don't say what the function does. I kinda think that the function name is obvious enough that you don't need this big comment at all.

Also, let's nix the "Ptr" from the parameter name, it's quite obvious that it's a pointer.

@@ +566,5 @@
>    bool IsPartOfApp();
>  
>  protected:
> +  // Idle timer used for function callbacks to notify
> +  // idle observers.

Nit: This can fit on one line probably

@@ +570,5 @@
> +  // idle observers.
> +  nsCOMPtr<nsITimer> mIdleTimer;
> +    
> +  // Idle fuzz factor time.
> +  // Idle observers are notified after idle fuzz factor time for security reasons.

Nit: This comment is kinda unhelpful... What security reasons? The reason for this shouldn't be a secret.

@@ +575,5 @@
> +  PRUint32 mIdleFuzzFactor;
> +  
> +  // This indicates if the back event was a direct user triggered event such as
> +  // a mouse, drag or key event.
> +  bool mAddBackEventFuzz;

I don't think you need this. In each place you set it you immediately call (well, you don't but you should!) FireBackStatusEvent. Let's just make this a parameter and lose the member entirely.

@@ +583,5 @@
> +  PRInt32 mIdleCallbackIndex;
> +
> +  // If false then the topic is "back"
> +  // If true then the topic is "idle"
> +  bool mIdleTopic;

I think we should rename this... "mCurrentlyIdle" perhaps? Calling it "mIdleTopic" makes me think it should be a string.

@@ +589,3 @@
>    friend class HashchangeCallback;
>    friend class nsBarProp;
> +  

Nit: Please revert.

@@ +712,5 @@
>  
> +  // Idle or back topic timer that is used when a fuzz factor has been applied 
> +  // when an "idle" or "back" topic notification has been received from the
> +  // idle service.
> +  static void IdleBackTimerCallback(nsITimer *aTimer, void *closure);

I think you should remove this and the next static method and have them live entirely in the cpp file. You can move all of the guts to your other methods.

@@ +736,5 @@
>    void     FireAbuseEvents(bool aBlocked, bool aWindow,
>                             const nsAString &aPopupURL,
>                             const nsAString &aPopupWindowName,
>                             const nsAString &aPopupWindowFeatures);
> +  

Nit: please revert

@@ +741,3 @@
>    void FireOfflineStatusEvent();
> +
> +  nsresult SetNextIdleObserverCallback();

Nit: This name doesn't really make sense, you're not really "setting" anything. You're "scheduling", I think.

@@ +741,4 @@
>    void FireOfflineStatusEvent();
> +
> +  nsresult SetNextIdleObserverCallback();
> +  nsresult GetIdleBackFuzzFactor(PRUint32* aRandNum);

Nit: How about "GetFuzzTimeMS"?

@@ +743,5 @@
> +  nsresult SetNextIdleObserverCallback();
> +  nsresult GetIdleBackFuzzFactor(PRUint32* aRandNum);
> +
> +  /* Function FireBackStatusEvent() fires the back event
> +   * with/without a fuzz factor. The fuzz factor is a random 

I think this comment should be removed. It's talking about a function that doesn't seem to exist any longer (the one with the same name has no params, and no indication of what would cause "with/without a fuzz factor").

@@ +753,5 @@
> +   * @note: This function will cancel any timer based function callback
> +   *        set for idle notification.
> +   */
> +  nsresult FireBackStatusEvent();
> +  nsresult FireIdleStatusEvent();

Nit: These methods aren't firing any events, they're scheduling a timer. Please rename.

@@ +754,5 @@
> +   *        set for idle notification.
> +   */
> +  nsresult FireBackStatusEvent();
> +  nsresult FireIdleStatusEvent();
> +  nsresult FireIdleBackEvent(const nsAString& aName);

Nit: This function isn't firing an event (like, a DOM event). Let's call this "NotifyIdleObservers".

@@ +756,5 @@
> +  nsresult FireBackStatusEvent();
> +  nsresult FireIdleStatusEvent();
> +  nsresult FireIdleBackEvent(const nsAString& aName);
> +  nsresult FindInsertIndexAt(nsIIdleObserver* aIdleObserver,
> +                             PRUint32* aInsertAtIndex);

This function needs a better name. Also, it shouldn't be possible for it to fail.

@@ +758,5 @@
> +  nsresult FireIdleBackEvent(const nsAString& aName);
> +  nsresult FindInsertIndexAt(nsIIdleObserver* aIdleObserver,
> +                             PRUint32* aInsertAtIndex);
> +  nsresult InsertIdleObserverAt(nsIIdleObserver* aIdleObserver,
> +                                PRUint32 aInsertAtIndex);

I don't think this function is complicated enough to need to exist. Please inline it in the one place you call it.

@@ +918,5 @@
>  
>    // Track what sorts of events we need to fire when thawed
>    bool                          mFireOfflineStatusChangeEventOnThaw : 1;
> +  bool                          mFireIdleStatusChangeEventOnThaw : 1;
> +  bool                          mFireBackStatusChangeEventOnThaw : 1;

We're not firing events, and there's no "Back" here either. You probably want "mNotifyIdleObserversIdleOnThaw" and "mNotifyIdleObserversActiveOnThaw"?

::: dom/base/nsPIDOMWindow.h
@@ +19,3 @@
>  #include "nsIURI.h"
> +#include "jsapi.h"
> +#include "nsIDOMNavigator.h"

Please revert this whole block. You only need to forward-declare nsIIdleObserver.

@@ +70,5 @@
>                      "active state is only maintained on outer windows");
>      mIsActive = aActive;
>    }
>  
> +  virtual nsresult RegisterIdleObserver(nsIIdleObserver* idleObserverPtr)

Nit: Please fix the name here, "aIdleObserver", and below.

@@ +72,5 @@
>    }
>  
> +  virtual nsresult RegisterIdleObserver(nsIIdleObserver* idleObserverPtr)
> +  {
> +    //Intentionally empty. Only implemented by nsGlobalWindow

Nit: Space after //. Below too.

::: dom/interfaces/base/nsIDOMNavigator.idl
@@ +82,5 @@
>  
> +  /** 
> +   * Navigator requests to add an idle observer to the existing window.
> +   */
> +  void addIdleObserver(in nsIIdleObserver idleObserverPtr);

Nit: Needs an "a" prefix, and no need for "ptr", e.g.:

  void addIdleObserver(in nsIIdleObserver aIdleObserver);

For Remove too.

@@ +121,5 @@
>    nsIDOMMozWakeLock requestWakeLock(in DOMString aTopic);
>  };
> +
> +[scriptable, uuid(e017de83-0aca-4683-99f1-926fbfb8a10b)]
> +interface nsIIdleObserver : nsISupports

This should be split into its own IDL file.

::: dom/interfaces/base/nsIDOMWindow.idl
@@ +51,5 @@
>     * Set/Get the name of this window.
>     *
>     * This attribute is "replaceable" in JavaScript
>     */
> +  attribute DOMString                                   name;

This entire file looks like whitespace changes only. Please revert.

::: modules/libpref/src/init/all.js
@@ +3520,5 @@
>  pref("full-screen-api.exit-on-deactivate", true);
>  pref("full-screen-api.pointer-lock.enabled", true);
>  
> +// DOM idle observers API
> +pref("idle-observers-api.enabled", true);

Let's prefix with "dom."

::: widget/cocoa/nsIdleServiceX.h
@@ +12,2 @@
>  public:
>    NS_DECL_ISUPPORTS

This needs to be NS_DECL_ISUPPORTS_INHERITED

::: widget/gtk2/nsWidgetFactory.cpp
@@ -108,5 @@
>  }
>  #endif
>  
>  #if defined(MOZ_X11)
> -NS_GENERIC_FACTORY_CONSTRUCTOR(nsIdleServiceGTK)

Why did you move this outside the #ifdef ? Will this compile if MOZ_X11 is not defined?

::: widget/nsGUIEvent.h
@@ +217,5 @@
>  
>  // Indicates a resize will occur
>  #define NS_BEFORERESIZE_EVENT            (NS_WINDOW_START + 66)
>  
> +//Indicates that the user is either idle or active

Nit: Space after //

::: widget/nsIIdleService.idl
@@ +66,5 @@
>      void removeIdleObserver(in nsIObserver observer, in unsigned long time);
>  };
>  
> +%{C++
> +  

Nit: Get rid of this extra newline

::: widget/nsIIdleServiceInternal.idl
@@ +8,5 @@
> +interface nsIIdleServiceInternal : nsIIdleService
> +{  
> + /**
> +   * Function that resets the idle time in the service, in other words it
> +   * sets the time for the last user interaction to now, or now-idleDelta

Nit: This comment doesn't really make sense, "now-idleDelta" looks like some hypenated non-word, and I don't know what subtracting could mean here. Let's do this:

 "Resets the idle time to the value specified."

@@ +13,5 @@
> +   *
> +   * @param idleDelta the time (in milliseconds) since the last user inter
> +   *                  action
> +   **/
> +  void resetIdleTimeOut(in unsigned long idleDeltaInMS);

Let's rename this "resetIdleTime". There "timeout" doesn't make sense here.

::: widget/windows/nsIdleServiceWin.h
@@ +18,5 @@
>  #define SAFE_COMPARE_EVEN_WITH_WRAPPING(A, B) (((int)((long)A - (long)B) & 0xFFFFFFFF))
>  #endif
>  
>  
>  class nsIdleServiceWin : public nsIdleService

Please add protected but empty constructor/destructor. E.g.:

  protected:
    nsIdleServiceWin() { }
    virtual ~nsIdleServiceWin() { }

Let's make sure all the other platfor implementations do this too (though of course some may have non-empty functions here).

@@ +26,4 @@
>  
>      bool PollIdleTime(PRUint32* aIdleTime);
>  
> +    static nsIdleServiceWin* GetDerivedInstance()

already_AddRefed, see below.

And there's no reason you need to call this "GetDerivedInstance", let's just keep it "GetInstance".

You need to do this for all the other platform implementations too.

::: widget/windows/nsWindow.cpp
@@ +6077,5 @@
>    }
>  
>    // Check that we now have the idle service.
>    if (mIdleService) {
> +    mIdleService->ResetIdleTimeOut(0);

Ok, so... You moved ResetIdleTimeOut to a completely new interface, yet this still compiles? How is that possible?

Ah, the way it works is that nsWindow.h has this:

  nsCOMPtr<nsIdleService> mIdleService;

That's totally broken. You need to change it to 'nsIIdleServiceInternal', and then you need to do that for all the other classes that do this. A simple mxr search shows that multiple implementations have copy/paste this brokenness.

::: widget/xpwidgets/nsIdleService.cpp
@@ +244,5 @@
> +nsIdleService*
> +nsIdleService::GetInstance()
> +{
> +  NS_IF_ADDREF(gIdleService);
> +  return gIdleService;

Once you change this to return an already_AddRefed (see below) this method should become:

  already_AddRefed<nsIdleService>
  nsIdleService::GetInstance()
  {
    nsRefPtr<nsIdleService> instance(gIdleService);
    return instance.forget();
  }

::: widget/xpwidgets/nsIdleService.h
@@ +101,5 @@
>    NS_IMETHOD RemoveIdleObserver(nsIObserver* aObserver, PRUint32 aIdleTime);
>    NS_IMETHOD GetIdleTime(PRUint32* idleTime);
>  
>    /**
>     * Function that resets the idle time in the service, in other words it

Nit: It's not a good idea to have duplicated comments. They tend to get out of sync over time. Please remove this.

@@ +112,3 @@
>  
>  protected:
> +  static nsIdleService* GetInstance();

In general you should be nervous about any method that returns a raw pointer. Does the getter call AddRef for you? Or is it meant to be a weak pointer? Without a comment here it's impossible to know.

In this particular case the method does call AddRef. Even better than a comment here, then, is to make the method return an |already_AddRefed<nsIdleService>|. That tells you what the method does as well as enforces that you assign the result into a nsRefPtr.

::: xpcom/glue/nsTObserverArray.h
@@ +145,5 @@
>      //
>      // Mutation methods
>      //
> +  
> +    // Prepend a given element to the given element index.

You're not prepending here, please fix this comment.

@@ +151,5 @@
> +    // @param item   The item to insert,
> +    // @return       A pointer to the newly inserted element, or a null on DOM
> +    template<class Item>
> +    elem_type *InsertElementAt(index_type aIndex, const Item& aItem) {
> +      return mArray.InsertElementAt(aIndex, aItem);

This is not enough, you also need to adjust the iterators (and in the next method below).
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #52)
> https://github.com/mozilla-b2g/gaia/pull/1731
> 
> I've come up Gaia fix for this. You can try with it or point to me if there
> is anything wrong.

Hi Tim,

So after much investigation and discussion with ben, looks like for now we'll have to rely on the front-end gaia fix for onidle = null and onactive = null. If you need to pass null for both onactive and onidle and then later on set the onactive and/or onidle to some callback function this will work too. The Idle API will still continue to function correctly with both callbacks being set to null. It will not crash. I have thoroughly tested this.

I will download your patch and apply it to b2g once I apply mine.
(In reply to bsurender from comment #54)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #52)
> > https://github.com/mozilla-b2g/gaia/pull/1731
> > 
> > I've come up Gaia fix for this. You can try with it or point to me if there
> > is anything wrong.
> 
> Hi Tim,
> 
> So after much investigation and discussion with ben, looks like for now
> we'll have to rely on the front-end gaia fix for onidle = null and onactive
> = null. If you need to pass null for both onactive and onidle and then later
> on set the onactive and/or onidle to some callback function this will work
> too. The Idle API will still continue to function correctly with both
> callbacks being set to null. It will not crash. I have thoroughly tested
> this.
> 
> I will download your patch and apply it to b2g once I apply mine.

Oops I kind of missed this message.

I don't quite follow you on this actually: the ScreenManager.idleObserver object will not change a bit after ScreenManager.init() -- I just define the idleObserver.onidle in the init() instead of statically to get the reference of |self| in the scope.

Or, are you trying to say that for ScreenManager.setIdleTimeout(), I don't really need to remove the original object and added it again to change the timeout?

PS While making this comment, I realized I didn't actually pass the idleObserver object into the API. That is fixed. Let me know if there is anything else need to change. I hope we can get this landed next week.
Please note that Vivien and have finished working on bug 738530. When this bug is fixed, please land *BOTH* patches to Gecko at the same time, or Gecko will break. Thanks.

https://bugzilla.mozilla.org/show_bug.cgi?id=738530#c7
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #55)
> (In reply to bsurender from comment #54)
> > (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #52)
> > > https://github.com/mozilla-b2g/gaia/pull/1731
> > > 
> > > I've come up Gaia fix for this. You can try with it or point to me if there
> > > is anything wrong.
> > 
> > Hi Tim,
> > 
> > So after much investigation and discussion with ben, looks like for now
> > we'll have to rely on the front-end gaia fix for onidle = null and onactive
> > = null. If you need to pass null for both onactive and onidle and then later
> > on set the onactive and/or onidle to some callback function this will work
> > too. The Idle API will still continue to function correctly with both
> > callbacks being set to null. It will not crash. I have thoroughly tested
> > this.
> > 
> > I will download your patch and apply it to b2g once I apply mine.
> 
> Oops I kind of missed this message.
> 
> I don't quite follow you on this actually: the ScreenManager.idleObserver
> object will not change a bit after ScreenManager.init() -- I just define the
> idleObserver.onidle in the init() instead of statically to get the reference
> of |self| in the scope.
> 
> Or, are you trying to say that for ScreenManager.setIdleTimeout(), I don't
> really need to remove the original object and added it again to change the
> timeout?

- You would have to remove the original object with the time it had when it was first added to the idle api and then add a new object with the new time. It doesn't matter if the onidle and onactive functions are the same as the original or were changed.

- For any idle observer object added to the idle api you can change the onidle and onactive callback functions anytime without having to remove the idle observer object that was originally added to the idle api.


> 
> PS While making this comment, I realized I didn't actually pass the
> idleObserver object into the API. That is fixed. Let me know if there is
> anything else need to change. I hope we can get this landed next week.
(In reply to bsurender from comment #57)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #55)
> > (In reply to bsurender from comment #54)
> > > (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #52)
> > > > https://github.com/mozilla-b2g/gaia/pull/1731
> > > > 
> > > > I've come up Gaia fix for this. You can try with it or point to me if there
> > > > is anything wrong.
> > > 
> > > Hi Tim,
> > > 
> > > So after much investigation and discussion with ben, looks like for now
> > > we'll have to rely on the front-end gaia fix for onidle = null and onactive
> > > = null. If you need to pass null for both onactive and onidle and then later
> > > on set the onactive and/or onidle to some callback function this will work
> > > too. The Idle API will still continue to function correctly with both
> > > callbacks being set to null. It will not crash. I have thoroughly tested
> > > this.
> > > 
> > > I will download your patch and apply it to b2g once I apply mine.
> > 
> > Oops I kind of missed this message.
> > 
> > I don't quite follow you on this actually: the ScreenManager.idleObserver
> > object will not change a bit after ScreenManager.init() -- I just define the
> > idleObserver.onidle in the init() instead of statically to get the reference
> > of |self| in the scope.
> > 
> > Or, are you trying to say that for ScreenManager.setIdleTimeout(), I don't
> > really need to remove the original object and added it again to change the
> > timeout?
> 
> - You would have to remove the original object with the time it had when it
> was first added to the idle api and then add a new object with the new time.
> It doesn't matter if the onidle and onactive functions are the same as the
> original or were changed.

Good, I am doing that right now on the branch.

> 
> - For any idle observer object added to the idle api you can change the
> onidle and onactive callback functions anytime without having to remove the
> idle observer object that was originally added to the idle api.

I am don't need to do that in Gaia but it's good to know.

> 
> 
> > 
> > PS While making this comment, I realized I didn't actually pass the
> > idleObserver object into the API. That is fixed. Let me know if there is
> > anything else need to change. I hope we can get this landed next week.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #56)
> Please note that Vivien and have finished working on bug 738530. When this
> bug is fixed, please land *BOTH* patches to Gecko at the same time, or Gecko
> will break. Thanks.
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=738530#c7

I can land both patches with the idle api but is there any reason for the patches needing to be landed with the idle api patch?
> please land *BOTH* patches to Gecko at the same time, or Gecko will break. Thanks.

If neither changeset builds on its own, then they should probably go into a single changeset so you don't break everyone trying to bisect....
(In reply to Boris Zbarsky (:bz) from comment #60)
> > please land *BOTH* patches to Gecko at the same time, or Gecko will break. Thanks.
> 
> If neither changeset builds on its own, then they should probably go into a
> single changeset so you don't break everyone trying to bisect....

Fair enough thanks.
Attached patch Idle API review 3 (obsolete) — Splinter Review
Ready for the review.
Attachment #631448 - Attachment is obsolete: true
Attachment #631448 - Flags: review?(bent.mozilla)
Attachment #637212 - Flags: review?(jst)
Attached patch Idle API patch after review (obsolete) — Splinter Review
Attachment #637212 - Attachment is obsolete: true
Attachment #637212 - Flags: review?(jst)
Attached patch Idle API patch after review (obsolete) — Splinter Review
Attachment #637417 - Attachment is obsolete: true
Attached patch idle api patch after review (obsolete) — Splinter Review
Attachment #637426 - Attachment is obsolete: true
Attached patch Idle API fix that landed. (obsolete) — Splinter Review
This is Bonnie's latest patch, with a couple of minor tweaks by myself. This just landed.
Attachment #637802 - Flags: review+
(In reply to bsurender from comment #59)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #56)
> I can land both patches with the idle api but is there any reason for the
> patches needing to be landed with the idle api patch?

After this bug I should say. not together.
https://hg.mozilla.org/mozilla-central/rev/b405f493e834
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Depends on: 770171
Fix for:

https://bugzilla.mozilla.org/show_bug.cgi?id=770172
https://bugzilla.mozilla.org/show_bug.cgi?id=770175
Attachment #637429 - Attachment is obsolete: true
Attachment #637802 - Attachment is obsolete: true
Attachment #638409 - Flags: review?(jst)
Attachment #638409 - Attachment is obsolete: true
Attachment #638409 - Flags: review?(jst)
Depends on: 770656
Depends on: 772316
Not sure if this is the place to ask but just a followup to the thread discussing if Idle API should be exposed to normal web pages. There was a move towards putting a permission on this API, but I haven't seen anything since.

(for reference https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.planning/u0KF9fGd48A)
Whiteboard: [sec-assigned:pauljt]
Flags: sec-review?(ptheriault)
Why don't we have any active patches listed on this bug for the check-ins happened?
Whiteboard: [sec-assigned:pauljt]
This API is certified apps only (see [1] and bug 780507)  which addresses any privacy concern related to web pages accessing this API. I don't think there is really any further to review here until such a time when this API is exposed to third-party apps.


[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#728
Flags: sec-review?(ptheriault) → sec-review+
Blocks: 878601
Blocks: 953312
Blocks: 953311
Blocks: 953282
No longer blocks: 953311
Depends on: 953311
No longer blocks: 953282, 953312
Depends on: 953282, 953312, 957585
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.