Note: There are a few cases of duplicates in user autocompletion which are being worked on.

refactor nsGlobalWindow::RunTimeout a bit

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Attachment #617956 - Flags: feedback?(bzbarsky)

Comment 2

5 years ago
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!
Attachment #617956 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 3

5 years ago
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.
Attachment #617956 - Attachment is obsolete: true
Attachment #625759 - Flags: review?(mounir)

Updated

5 years ago
Assignee: nobody → nfroyd
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
(Assignee)

Comment 5

5 years ago
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?
Attachment #625759 - Attachment is obsolete: true
Attachment #625759 - Flags: review?(mounir)
Attachment #626002 - Flags: review?(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.
Attachment #626002 - Flags: review?(mounir) → review?(bzbarsky)

Comment 7

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

Comment 8

5 years ago
> I guess local style for dom/ is aspirational

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

Comment 9

5 years ago
Created attachment 626415 [details] [diff] [review]
interdiff between original (attach# 617956) and new (attach#626002)

Comment 10

5 years ago
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!
Attachment #626002 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3eed3675f5f
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/e3eed3675f5f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16

Updated

5 years ago
Blocks: 763247
You need to log in before you can comment on or make changes to this bug.