1357731 - Consider removing Timer.jsm

Status: NEW

(Firefox :: General, enhancement)

Description

[Marco Castelluccio [:marco]]

Timer.jsm is a really simple wrapper around nsITimer, it could be removed.

Comment 1

[Florian Quèze [:florian]]

Per the discussion in bug 1353731 comment 6 through 8, it seems exposing setTimeout and friends using importGlobalProperties would be the best way forward.

Needinfo on bholley and froydnj to figure out who would be the best person to handle this.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Florian Quèze [:florian] [:flo] from comment #1)
> Per the discussion in bug 1353731 comment 6 through 8, it seems exposing
> setTimeout and friends using importGlobalProperties would be the best way
> forward.

One thing that's tricky about this is that the current setTimeout logic is all tangled up with nsGlobalWindow, and generalizing it to the point that it could be used in other globals is non-trivial to say the least. I should have been more explicit about this in bug 1353731 - my comment there was just to say that anything new exposed on sandboxes should be opt-in, orthogonal to how hard it would be to expose the given thing.

> 
> Needinfo on bholley and froydnj to figure out who would be the best person
> to handle this.

These are busy times, and I don't think that either I or Nathan will have cycles for this anytime soon. Unless this is extremely hard priority, you will likely find it difficult to find an XPConnect-capable hacker to work on this in the 57 timeframe.

Comment 3

[(no longer active)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2)
> (In reply to Florian Quèze [:florian] [:flo] from comment #1)
> > Per the discussion in bug 1353731 comment 6 through 8, it seems exposing
> > setTimeout and friends using importGlobalProperties would be the best way
> > forward.
> 
> One thing that's tricky about this is that the current setTimeout logic is
> all tangled up with nsGlobalWindow, and generalizing it to the point that it
> could be used in other globals is non-trivial to say the least. I should
> have been more explicit about this in bug 1353731 - my comment there was
> just to say that anything new exposed on sandboxes should be opt-in,
> orthogonal to how hard it would be to expose the given thing.

This has actually recently been improved somewhat, most of this has been refactored into the TimeoutManager class now.  I didn't pay a lot of attention to make it completely independent of nsGlobalWindow.

None of this of course changes Bobby's comment about the complexities of dealing with the globals, but with some work we may actually be able to make TimeoutManager fully standalone and expose setTimout/setInterval to JSMs based on that.  Assuming we had a solution for the other issues involved.  :-)

Comment 4

[Florian Quèze [:florian]]

Andreas, is this bug something you could help with? I'm mostly asking about the "exposing setTimeout and friends using importGlobalProperties" part. I'm happy to do the needed changes to our JS code if we can have a good replacement for the current Timer.jsm module.

Comment 5

[Andreas Farre [:farre]]

Not sure that I have the time either, but I had a look at what needs to be done.

Things TimeoutManager relies on nsGlobalWindow for:

* IsBackgroundInternal
* IsChromeWindow
* IsDocumentLoaded
* InnerObjectsFreed
* IsInnerWindow
* TimeoutManager

Used mainly to control throttling (start, resume, ...).

* AsInner

Used to be able to call IsPlayingAudio, InnerObjectsFreed,
IsDocumentLoaded, TimeoutManager. These all have to do with throttling.

* IsPlayingAudio
* WindowID

Called from IsActive only, also a throttling thing.

* GetExtantDoc

Check if there is a document at all before setting timeout.

* IsSuspended
* IsFrozen

Called all over the place, mainly to determine scheduling (timeouts and executor).

* GetPopupControlState

Used to block popups

* GetContextInternal

Get the script context to run the script with. Differs from nsIScriptGlobalObject::GetContext in that it also returns a script context for inner window.

* RunTimeoutHandler

Runs the timeout.

* EventTargetFor

Used to implement TimeoutManager::EventTarget.

Comment 6

[Kris Maglione [:kmag]]

I suspect we can just add a few native helpers that wraps nsITimer in the same way that Timer.jsm does, and not worry about the extra global management and suspension complexity that the DOM functions add.

Using native stubs would have the benefit of not requiring that we create nsITimer wrappers (which are expensive) every time we create a new timer. It has the disadvantage of likely eagerly creating those global properties and function wrappers in every scope that uses them, if we're not careful, since importGlobalProperties eagerly imports all of its globals, and even defineLazyGlobalGetters needs to eagerly define getter properties.

I'd generally worry about encouraging people to use nsITimer directly. It's too easy to make mistakes, and either fail to keep the nsITimer instance alive (and therefore have your timer fail if GC happens before it fires), or to fail to clean it up your strong references after you're done with it (and therefore to leak).