615921 - timer module should be in add-on kit

Status: RESOLVED FIXED

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

Description

[Dietrich Ayala (:dietrich)]

timer is an api that's used heavily by add-on code, not just module code.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Move code, tests and doc

Comment 2

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

Alex I don't think we want to have `timer-e10s-adapter.js` laying around together with high level modules, specially when we are not doing e10s stuff in this cycle. Also leaving it in the api-utils is not an option cause then we will have circular package dependency.

Myk can we remove timer-e10s-adapter since we are not shipping in 1.0, also we can add it later if necessary.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #2)
> Myk can we remove timer-e10s-adapter since we are not shipping in 1.0, also we
> can add it later if necessary.

Yup, sounds like a plan, go right ahead!

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Move code, tests and doc. Delete e10s adapter.

Comment 5

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

Comment on attachment 526007 [details] [diff] [review]
Move code, tests and doc. Delete e10s adapter.

Sorry I still can not accept this patch as lot's of low level modules in api-utils depend on 'timer'. Moving timer to the addon-kit will introduce circular package dependency.

Here is the list of modules in api-utils that require timer:

lib/find-tests-e10s-adapter.js|38 col 15| var timer = require("timer");
lib/hidden-frame.js|44 col 15| const timer = require("timer");
lib/tab-browser.js|76 col 11| require("timer").setTimeout(function () {
lib/tab-browser.js|573 col 7| require("timer").setTimeout(function() {
lib/unit-test.js|39 col 13| var timer = require("timer");
lib/utils/function.js|38 col 22| var { setTimeout } = require("timer");
lib/windows/loader.js|39 col 24| { setTimeout } = require("timer"),
tests/test-tab-browser.js|37 col 13| var timer = require("timer");
tests/test-tab-browser.js|55 col 11| require("timer").setTimeout(function () {
tests/test-tab-browser.js|70 col 3| require("timer").setTimeout(function() {
tests/test-tab-browser.js|156 col 9| require("timer").setTimeout(function() {
tests/test-tab-browser.js|269 col 11| require("timer").setTimeout(function() test.done(), 0);
tests/test-tab-browser.js|379 col 7| require("timer").setTimeout(function() {
tests/test-tab-browser.js|482 col 9| require("timer").setTimeout(function() {
tests/test-tab.js|55 col 19| let timer = require("timer");
tests/test-window-utils.js|2 col 13| var timer = require("timer");
tests/test-xhr.js|2 col 13| var timer = require("timer");
lib/content/worker.js|45 col 15| const timer = require('timer');
tests/test-e10s.js|2 col 13| var timer = require('timer');

Comment 6

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

I think only option left is making timer module in the addon-kit that just proxies to this one. But then we are facing an issue I have being rising: what does require("timer") will have to return addon-kit/timer or api-utils/timer ?

Comment 7

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #6)
> I think only option left is making timer module in the addon-kit that just
> proxies to this one. But then we are facing an issue I have being rising: what
> does require("timer") will have to return addon-kit/timer or api-utils/timer ?

require("timer") will return api-utils/timer inside api-utils.  It will return addon-kit/timer inside other packages.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

And if you want to make this extra clear, you can move api-utils' timer-related exports to a differently-named utility module.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Just move doc

I tried to implement a fake module in addon-kit:
  exports = require("timer");
But I got this message multiple times:
warning: require(timer) (called from resource://addon-kit-addon-kit-tests/test-t
imer.js) is loading resource://addon-kit-api-utils-lib/timer.js, but is supposed
 to be loading resource://addon-kit-addon-kit-lib/timer.js

Then we still can't do such thing:
  exports = require("api-utils/timer");

And renaming timer in api-utils is really confusing because we will have to use require("timer") in high level and require("timer-utils") [whatever the name] in low level. Althought it's exactly the same and having to require timer to use setTimeout/setInterval is already painfull itself. I don't know any JS environnement that doesn't have access to these function in globals ?

I think we are trying to do something over-complicated, so I propose to only move the documentation.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Brian has been working on making `require("api-utils/timer")` work.

Brian: when will that be available?

Comment 11

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

Comment on attachment 526713 [details] [diff] [review]
Just move doc

I r- this review as Myk disliked idea of moving docs only.

Comment 12

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

I wonder if we can rename timer to timers ? The later one is the name for this module in nodejs. This will be helpful for people coming from node and to people developing cross engine libs.

Comment 13

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

I have discussed comment above with myk and we have divided to do following:

1. Create high level module 'timers' in 'addon-kit' that will be a proxy to an existing low level module 'timer' in 'api-utils'.
2. Move timer docs to "addon-kit" with a name "timers".

This should make it possible to use require("timer") and require("timers"). Also we will encourage using later one.

Comment 14

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

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

Pointer to Github pull-request

Comment 15

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

Please note that we would like to use `require("api-utils/timer")` instead of the `require("timer")` but all id transformations will be handled by a separate bug and most likely will be done by a script.

Comment 16

[Dietrich Ayala (:dietrich)]

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

i think we need to update the example add-ons, and any examples in the add-on kit docs that use timers. ready to r+ once those are done.

Comment 17

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

Only case we refered to timer in docs / examples was in widget docs and that pull request was changing it to timers. So I'm landing this as is.

Comment 18

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

Landed https://github.com/mozilla/addon-sdk/commit/79c178697dc74175a1f08e5b0d3fdee65efcd482

Comment 19

[Dietrich Ayala (:dietrich)]

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

great, thanks for checking that. post-facto r+!

Comment 20

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

Reopening, as links in docs have not been updated.

Comment 21

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

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

Pointer to Github pull-request

Comment 22

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

https://github.com/mozilla/addon-sdk/commit/f02e68abd4447df0ee2ae1e0a69370b523e676b1

Comment 23

[Will Bamberg [:wbamberg]]

Attachment: Missed a referenced to timer

Comment 24

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

Comment on attachment 535082 [details] [diff] [review]
Missed a referenced to timer

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

Looks good, thanks!

Comment 25

[Will Bamberg [:wbamberg]]

Fixed by https://github.com/mozilla/addon-sdk/commit/243ba57e1d741eeacbe25e0f7a2e0580629577f0