Last Comment Bug 748464 - refactor nsGlobalWindow::RunTimeout a bit
: refactor nsGlobalWindow::RunTimeout a bit
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Nathan Froyd [:froydnj]
:
: Andrew Overholt [:overholt]
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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 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 Nathan Froyd [:froydnj] 2012-04-24 11:18:56 PDT
Created attachment 617956 [details] [diff] [review]
patch

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 Boris Zbarsky [:bz] (still a bit busy) 2012-05-11 21:55:37 PDT
Comment on attachment 617956 [details] [diff] [review]
patch

>+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 Nathan Froyd [:froydnj] 2012-05-21 14:08:37 PDT
Created attachment 625759 [details] [diff] [review]
patch

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

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;

Same

@@ +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 Nathan Froyd [:froydnj] 2012-05-22 07:37:56 PDT
Created attachment 626002 [details] [diff] [review]
patch

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

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

Redirecting this to bz because I've never touched nor read that code before.
Comment 7 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 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 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 Boris Zbarsky [:bz] (still a bit busy) 2012-05-23 08:47:43 PDT
Comment on attachment 626002 [details] [diff] [review]
patch

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

r=me with that.  Thanks for the interdiff!
Comment 12 Ed Morley [:emorley] 2012-06-06 08:42:23 PDT
https://hg.mozilla.org/mozilla-central/rev/e3eed3675f5f

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