Last Comment Bug 771967 - OS Idle API active to brief idle to active notification bug fix
: OS Idle API active to brief idle to active notification bug fix
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: bsurender
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-08 19:55 PDT by bsurender
Modified: 2012-08-09 08:22 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP active to active bug fix. waiting for try results. (14.43 KB, patch)
2012-07-08 19:55 PDT, bsurender
jst: review+
Details | Diff | Splinter Review
OS idle api active to briefly idle to active notification fix. (14.61 KB, patch)
2012-07-13 15:43 PDT, bsurender
no flags Details | Diff | Splinter Review

Description bsurender 2012-07-08 19:55:50 PDT
Created attachment 640123 [details] [diff] [review]
WIP active to active bug fix. waiting for try results.

Bug fix for case below:

When the user is active and then becomes idle, idle observers are notified. But if the user becomes active again before some or all idle observers are notified then active notifications need not be sent to those idle observers. 

Changeset uploaded to try at https://tbpl.mozilla.org/?tree=Try&rev=1924f96218bc

Will request a review once the try results are correct.
Comment 1 bsurender 2012-07-09 09:39:04 PDT
Clean green for the most part.
Oranges and purples were re-run and are now green.

https://tbpl.mozilla.org/?tree=Try&rev=1924f96218bc
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2012-07-13 14:10:42 PDT
Comment on attachment 640123 [details] [diff] [review]
WIP active to active bug fix. waiting for try results.

+nsGlobalWindow::NotifyIdleObserver(IdleObserverHolder* aIdleObserverHolder,
                                    bool aCallOnidle)
 {
+  MOZ_ASSERT(aIdleObserverHolder);
+  aIdleObserverHolder->mPrevNotificationIdle = aCallOnidle ? true : false;

No need to check the boolean aCallOnidle here, just assign it directly.

- In nsGlobalWindow::HandleIdleObserverCallback():

+  IdleObserverHolder* idleObserver =
+    &(mIdleObservers.ElementAt(mIdleCallbackIndex));
+  NotifyIdleObserver(idleObserver, true);

Make idleObserver be a reference here and drop the extra parentheses. Pass the address of idleObserver to NotifyIdleObserver(). That way you don't take the address of a return value, which just looks a bit odd (it works, but using a reference here is IMO cleaner). Alternatively you could even make NotifyIdleObserver() take a reference instead of a pointer, assuming you always have an observer holder to pass in, which you must since you assert it's never null.

- In nsGlobalWindow::HandleIdleActiveEvent()

+    IdleObserverHolder* idleObserver = &(iter.GetNext());

Same deal, use a reference, pass the address of the reference below.

- In nsGlobalWindow::RegisterIdleObserver(nsI

+    IdleObserverHolder* idleObserver =
+      &(mIdleObservers.ElementAt(insertAtIndex));

Same here, Use a reference

- In nsGlobalWindow.h:

   IdleObserverHolder(const IdleObserverHolder& aOtherIdleObserver)
-    : mIdleObserver(aOtherIdleObserver.mIdleObserver), mTimeInS(aOtherIdleObserver.mTimeInS)
+    : mIdleObserver(aOtherIdleObserver.mIdleObserver), mTimeInS(aOtherIdleObserver.mTimeInS), 
+      mPrevNotificationIdle(aOtherIdleObserver.mPrevNotificationIdle)

Maybe rename "aOtherIdleObserver" here to simply "aOther" (fairly common practice in copy constructors)? Saves on typing, makes this easier to read...

r=jst with that!
Comment 3 bsurender 2012-07-13 15:43:16 PDT
Created attachment 642106 [details] [diff] [review]
OS idle api active to briefly idle to active notification fix.
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2012-07-14 21:41:10 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/23225d8cc529
Comment 5 Ryan VanderMeulen [:RyanVM] 2012-07-15 10:19:20 PDT
https://hg.mozilla.org/mozilla-central/rev/23225d8cc529

Note You need to log in before you can comment on or make changes to this bug.