Last Comment Bug 748464 - refactor nsGlobalWindow::RunTimeout a bit
: refactor nsGlobalWindow::RunTimeout a bit
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla16
Assigned To: Nathan Froyd [:froydnj]
: Andrew Overholt [:overholt]
Depends on:
Blocks: 763247
  Show dependency treegraph
Reported: 2012-04-24 11:16 PDT by Nathan Froyd [:froydnj]
Modified: 2012-06-11 09:31 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (16.40 KB, patch)
2012-04-24 11:18 PDT, Nathan Froyd [:froydnj]
bzbarsky: feedback+
Details | Diff | Splinter Review
patch (16.29 KB, patch)
2012-05-21 14:08 PDT, Nathan Froyd [:froydnj]
no flags Details | Diff | Splinter Review
patch (16.56 KB, patch)
2012-05-22 07:37 PDT, Nathan Froyd [:froydnj]
bzbarsky: review+
Details | Diff | Splinter Review
interdiff between original (attach# 617956) and new (attach#626002) (7.40 KB, patch)
2012-05-23 06:33 PDT, Nathan Froyd [:froydnj]
no flags Details | Diff | Splinter Review

Description User image Nathan Froyd [:froydnj] 2012-04-24 11:16:15 PDT
For bug 715376, I *think* I need the ability to run timeouts outside of RunTimeout.  (I haven't totally figured out where everything goes yet, though.)  Some moving things around is helpful for doing that.

Independent of those concerns, though, RunTimeout is a pretty big function and breaking it up is helpful.
Comment 1 User image Nathan Froyd [:froydnj] 2012-04-24 11:18:56 PDT
Created attachment 617956 [details] [diff] [review]

I think the new RescheduleTimeout function is especially helpful at clarifying the logic for interval timeouts.

Asking for feedback, rather than review, to see if this sort of thing is even desirable to start with.
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2012-05-11 21:55:37 PDT
Comment on attachment 617956 [details] [diff] [review]

>+nsGlobalWindow::RescheduleTimeout(nsTimeout *aTimeout, const TimeStamp &now)
>+    if (aTimeout->mTimer) {
>+        // The timeout still has an OS timer, and it's not an
>+        // interval, that means that the OS timer could still fire (if
>+        // it didn't already, i.e. aTimeout == timeout), cancel the OS

There is no |timeout| in this method.  Just take out the parenthetical?  And fix the indent on the comment, please.

>+  // If we're running pending timeouts because they've been temporarily
>+  // disabled (!aTimeout), set the next interval to be relative to "now",
>+  // and not to when the timeout that was pending should have fired.
>+  TimeStamp firingTime;
>+  if (!aTimeout)

No, this is wrong.  In the caller, where the code and comment came from, aTimeout is the thing passed to RunTimeout; that's what was being null-checked originally.  But in this method, aTimeout is the |timeout| from RunTimeout, which in particular is never null here.  So this code is no longer doing what it should.  Please fix that.

>+  bool RescheduleTimeout(nsTimeout *aTimeout, const TimeStamp &now);

Please document the meaning of the return value.

Sorry for the lag here; apart from the above nits and firingTime issue, this looks good!
Comment 3 User image Nathan Froyd [:froydnj] 2012-05-21 14:08:37 PDT
Created attachment 625759 [details] [diff] [review]

Think I fixed everything bz pointed out; bouncing r? to someone different for a second pair of eyes.
Comment 4 User image :Ms2ger (⌚ UTC+1/+2) 2012-05-21 14:23:47 PDT
Comment on attachment 625759 [details] [diff] [review]

Review of attachment 625759 [details] [diff] [review]:

::: dom/base/nsGlobalWindow.cpp
@@ +8801,5 @@
>  }
> +void
> +nsGlobalWindow::RunTimeoutHandler(nsTimeout *aTimeout,
> +                                  nsIScriptContext *aScx)

* on the left

@@ +8803,5 @@
> +void
> +nsGlobalWindow::RunTimeoutHandler(nsTimeout *aTimeout,
> +                                  nsIScriptContext *aScx)
> +{
> +  nsTimeout *last_running_timeout = mRunningTimeout;


@@ +8819,5 @@
> +  aTimeout->mPopupState = openAbused;
> +
> +  // Hold on to the timeout in case mExpr or mFunObj releases its
> +  // doc.
> +  aTimeout->AddRef();

Manual AddRefs are bad. Please use a smart pointer

@@ +8909,5 @@
> +  TimeStamp firingTime;
> +  if (aRunningPendingTimeouts)
> +    firingTime = now + nextInterval;
> +  else
> +    firingTime = aTimeout->mWhen + nextInterval;

2 × {}

@@ +9115,5 @@
>      // |timeout| may be the last reference to the timeout so check if
>      // it was cleared before releasing it.
>      bool timeout_was_cleared = timeout->mCleared;
>      timeout->Release();

This release should move into the helper
Comment 5 User image Nathan Froyd [:froydnj] 2012-05-22 07:37:56 PDT
Created attachment 626002 [details] [diff] [review]

Updated with Ms2ger's concerns.  I guess local style for dom/ is aspirational, rather than what one finds in nearby code?
Comment 6 User image Mounir Lamouri (:mounir) 2012-05-22 09:55:02 PDT
Comment on attachment 626002 [details] [diff] [review]

Review of attachment 626002 [details] [diff] [review]:

Redirecting this to bz because I've never touched nor read that code before.
Comment 7 User image Boris Zbarsky [:bz] (still a bit busy) 2012-05-22 10:08:22 PDT
Any chance of an interdiff from the last thing I reviewed?
Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2012-05-22 10:09:08 PDT
> I guess local style for dom/ is aspirational

More precisely, it's a matter of some recent disagreement... ;)
Comment 9 User image Nathan Froyd [:froydnj] 2012-05-23 06:33:07 PDT
Created attachment 626415 [details] [diff] [review]
interdiff between original (attach# 617956) and new (attach#626002)
Comment 10 User image Boris Zbarsky [:bz] (still a bit busy) 2012-05-23 08:47:43 PDT
Comment on attachment 626002 [details] [diff] [review]

The "timeout->AddRef();" in RunTimeoutHandler should go away.  Otherwise this leaks.

r=me with that.  Thanks for the interdiff!
Comment 12 User image Ed Morley [:emorley] 2012-06-06 08:42:23 PDT

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