724772 - make setTimeout / setInterval globals

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect, P2)

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/780/files

Pointer to Github pull-request

Comment 2

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Only issue I have with this is that other users of loader will get surprised that they won't have these timer functions making code reuse between add-ons and firefox features harder.

I could probably convinced that this is ok, but so far I'm not.

Alternative could be to make timers part of loader, although that feels like an overkill to me.

This makes me wonder how harmony modules are going to go about it. At first I remember they were supposed to be in the module, but later it was reconsidered. Eddy should know it better, I'd like to know if in ES.next timer functions are globals in module scope and what happens with registered timers if loader is destroyed (Although I don't think it can be destroyed in ES.next).

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 712396 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/780/files

We can re-request review if we decide it's a way to go, but first lets have a conversation.

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1184

Pointer to Github pull-request

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

I'm guessing you meant to needinfo me Irakli?

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 791759 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1184

I did meant needinfo on Eddy since he was involved in implementing ES6 modules so he should be aware what's the plan for the timer functions there. Erik I'm sorry to be rejecting this once again, but my last comments still stand, I don't want to expose globals that standard is not going to be exposing and last decision I was aware of was to not include timers into module global scope, since it's not even part of JS but rather part of DOM APIs. Plan was to make them importable in form of standard built-in module that browsers will expose. If this still a plan I would rather not expose timers since it will only make it harder for us to switch to standard modules when they'll finally become available.

Could you please explain why you think it's important before doing any further work so we avoid spending more of your time.

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 791759 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1184

Thanks!
It would be great to test unload as well though.

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 791759 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1184

Oops I mixed up tabs, putting back r- for reasons commented earlier

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

Alea iacta est

NodeJS already supports these as globals, and makes no mention of the option to require('timers'), so I don't think there is any need for discussion.

Anyhow I don't know why I'm being needinfo'd..

Comment 10

[Eddy Bruel [:ejpbruel]]

I just talked to jorendorff. He said he will comment on this, so I'm clearing the needinfo flag.

Comment 11

[Jason Orendorff [:jorendorff]]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #2)
> [...] I'd like to know if in ES.next
> timer functions are globals in module scope and what happens with registered
> timers if loader is destroyed (Although I don't think it can be destroyed in
> ES.next).

In ES.next, modules can see all the globals.

So if your global object has a setTimeout function, sure, modules loaded into that global can see that. But if not, you would have to import it from somewhere in order to get access to it; the module spec won't say anything special about setTimeout in particular.

Comment 12

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 791759 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1184

re-requesting a review now that we have feedback.

Comment 13

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 791759 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1184

Erik, it feels like this does not really belongs in a timer domain. I think it would make most sense to move timers module and make it loadable either as js module or JSM.

I think it would make sense to add an option like `globalTimers` to a loader & unless it's `false` load timers and expose them as globals.

Comment 14

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #13)
> Comment on attachment 791759 [details]
> Pointer to Github pull request:
> https://github.com/mozilla/addon-sdk/pull/1184
> 
> Erik, it feels like this does not really belongs in a timer domain. I think
> it would make most sense to move timers module and make it loadable either
> as js module or JSM.
> 
> I think it would make sense to add an option like `globalTimers` to a loader
> & unless it's `false` load timers and expose them as globals.

This is the last change that you want?  I'd like the next time that I rewrite this to be the last..

Comment 15

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #14)
> (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> #13)
> > Comment on attachment 791759 [details]
> > Pointer to Github pull request:
> > https://github.com/mozilla/addon-sdk/pull/1184
> > 
> > Erik, it feels like this does not really belongs in a timer domain. I think
> > it would make most sense to move timers module and make it loadable either
> > as js module or JSM.
> > 
> > I think it would make sense to add an option like `globalTimers` to a loader
> > & unless it's `false` load timers and expose them as globals.
> 
> This is the last change that you want?  I'd like the next time that I
> rewrite this to be the last..

Erik I can promise next review will be positive. I just would like to reduce dependencies between
different components as such dependencies will be hard to deal with when switching to multiprocess
architecture.

It maybe best to hold on with this changes until we're done with other loader changes so we'll have
more clear picture.
clear

Comment 16

[Erik Vold [:erikvold] (please needinfo? me)]

I wanted to try to take a look at this, but I'm not sure that I will be able to now.

Comment 17

[Erik Vold [:erikvold] (please needinfo? me)]

I'm going to try this one again..

Comment 18

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562