Closed Bug 615921 Opened 11 years ago Closed 11 years ago

timer module should be in add-on kit

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dietrich, Assigned: irakli)

Details

Attachments

(4 files, 2 obsolete files)

timer is an api that's used heavily by add-on code, not just module code.
OS: Linux → All
Priority: -- → P1
Hardware: x86 → All
Target Milestone: --- → 1.0
Attached patch Move code, tests and doc (obsolete) — Splinter Review
Assignee: nobody → poirot.alex
Attachment #525681 - Flags: review?(rFobic)
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.
(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!
Attachment #525681 - Attachment is obsolete: true
Attachment #526007 - Flags: review?(rFobic)
Attachment #525681 - Flags: review?(rFobic)
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');
Attachment #526007 - Flags: review-
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 ?
(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.
And if you want to make this extra clear, you can move api-utils' timer-related exports to a differently-named utility module.
Attachment #526007 - Flags: review?(rFobic)
Attached patch Just move docSplinter Review
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.
Attachment #526007 - Attachment is obsolete: true
Attachment #526713 - Flags: review?(rFobic)
Brian has been working on making `require("api-utils/timer")` work.

Brian: when will that be available?
Comment on attachment 526713 [details] [diff] [review]
Just move doc

I r- this review as Myk disliked idea of moving docs only.
Attachment #526713 - Flags: review?(rFobic) → review-
Depends on: 654983
Assignee: poirot.alex → rFobic
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.
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.
Attachment #532911 - Flags: review?(dietrich)
No longer depends on: 654983
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 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.
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.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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+!
Attachment #532911 - Flags: review?(dietrich) → review+
Reopening, as links in docs have not been updated.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #534798 - Flags: review?(wbamberg)
Attachment #534798 - Flags: review?(wbamberg) → review+
https://github.com/mozilla/addon-sdk/commit/f02e68abd4447df0ee2ae1e0a69370b523e676b1
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 535082 [details] [diff] [review]
Missed a referenced to timer

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

Looks good, thanks!
Attachment #535082 - Flags: review?(rFobic) → review+
Fixed by https://github.com/mozilla/addon-sdk/commit/243ba57e1d741eeacbe25e0f7a2e0580629577f0
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.