748464 - refactor nsGlobalWindow::RunTimeout a bit

Status: RESOLVED FIXED

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

Description

[Nathan Froyd [:froydnj]]

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]]

Attachment: 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 [:bzbarsky]]

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]]

Attachment: patch

Think I fixed everything bz pointed out; bouncing r? to someone different for a second pair of eyes.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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]]

Attachment: 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)]

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 [:bzbarsky]]

Any chance of an interdiff from the last thing I reviewed?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I guess local style for dom/ is aspirational

More precisely, it's a matter of some recent disagreement... ;)

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: interdiff between original (attach# 617956) and new (attach#626002)

Comment 10

[Boris Zbarsky [:bzbarsky]]

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 11

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3eed3675f5f

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e3eed3675f5f