840360 - Extract the setTimeout implementation from /browser/devtools/shared/Browser.jsm into a new Timer.jsm in toolkit

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Browser.jsm contains a setTimeout implementation that can be used without a DOM window.  I have need of such a thing, and so did several other developers I talked to on IRC.  This patch splits that implementation out into a new module in Toolkit.

Note: As far as I can tell, gcli.jsm (the only consumer of Browser.jsm) does not actually use this setTimeout implementation.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 712717 [details] [diff] [review]
patch

I think I prefer the implementation in reftest-content.js (references the id via closure rather than iterating over all timers when they fire, shares code with clearTimeout). Alternatively I think you could just store the id as an expando on the timer object.

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #1)
> I think I prefer the implementation in reftest-content.js 

Okay!  Here it is, with some fairly mechanical changes to match toolkit JS style.

By the way, Splinter cannot handle the "hg cp" in this patch at all; you might want to read the raw diff and/or apply it locally to see the actual changes.

Comment 3

[Joe Walker [:jwalker]]

Comment on attachment 712803 [details] [diff] [review]
patch v2

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

Thanks Matt.

There were 2 thoughts behind Browser.jsm; one was overcoming the learning hurdle of places in which the web platform was a superset of the browser platform. The other was in helping the import of code designed for the web.

So I think Browser.jsm should either not exist (we don't care about the ideas above) or it should import and re-export setTimeout/clearTimeout from Timer.jsm. Right now, there's not much point to it. I think adding in an import/re-export is both simplest and doesn't involve changing anything conceptually, so that's probably the right thing to do.

Comment 4

[[e-mail]]

Comment on attachment 712803 [details] [diff] [review]
patch v2

>+  let id = gNextTimeoutId++;
Nit: Window timeout ids are non-zero.

>+  timer.initWithCallback({
>+    notify: function notify_callback() {
>+        clearTimeout(id);
>+        aCallback();
>+      }
>+    },
>+    aMilliseconds,
>+    timer.TYPE_ONE_SHOT);
Weird-looking indentation there ;-) Anyway, nsITimerCallback is a function interface so since you're already using function-style scoping you might as well pass the notify_callback function itself directly.

Nit: clearTimeout is overkill, gTimeoutTable.delete(id) suffices.

>+  let timer = gTimeoutTable.get(aId);
[Could use gTimeoutTable.has(aId) here]

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 712803 [details] [diff] [review]
patch v2

Sorry for lag.  Not for me to review here since it's my code originally ;).

Please be sure to fix the ++gNextTimeoutId issue.  r=me with that.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> Comment on attachment 712803 [details] [diff] [review]
> patch v2
> 
> Sorry for lag.  Not for me to review here since it's my code originally ;).

*Not much

Comment 7

[Matt Brubeck (:mbrubeck)]

Landed with review comments addressed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8202e140fd8

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backed out for timeouts in browser_browser_basic.js
https://hg.mozilla.org/integration/mozilla-inbound/rev/2da4ecd0b47d

https://tbpl.mozilla.org/php/getParsedLog.php?id=20075263&tree=Mozilla-Inbound

12:12:25     INFO -  TEST-START | chrome://mochitests/content/browser/browser/devtools/shared/test/browser_browser_basic.js
12:12:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/shared/test/browser_browser_basic.js | Node correctly defined
12:12:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/shared/test/browser_browser_basic.js | HTMLElement correctly defined
12:12:25     INFO -  MOZ_EVENT_TRACE sample 1361823145986 544
12:12:26     INFO -  MOZ_EVENT_TRACE sample 1361823146112 37
12:12:26     INFO -  MOZ_EVENT_TRACE sample 1361823146258 36
12:12:26     INFO -  MOZ_EVENT_TRACE sample 1361823146464 95
12:12:26     INFO -  MOZ_EVENT_TRACE sample 1361823146602 39
12:12:26     INFO -  MOZ_EVENT_TRACE sample 1361823146697 95
12:12:26     INFO -  MOZ_EVENT_TRACE sample 1361823146768 59
12:12:26     INFO -  MOZ_EVENT_TRACE sample 1361823146817 38
12:12:26     INFO -  MOZ_EVENT_TRACE sample 1361823146860 43
12:12:28     INFO -  MOZ_EVENT_TRACE sample 1361823148548 214
12:12:32     INFO -  MOZ_EVENT_TRACE sample 1361823152579 20
12:12:32     INFO -  MOZ_EVENT_TRACE sample 1361823152718 40
12:12:32     INFO -  MOZ_EVENT_TRACE sample 1361823152964 36
12:12:33     INFO -  MOZ_EVENT_TRACE sample 1361823153101 39
12:12:33     INFO -  MOZ_EVENT_TRACE sample 1361823153344 143
12:12:33     INFO -  MOZ_EVENT_TRACE sample 1361823153469 25
12:12:33     INFO -  MOZ_EVENT_TRACE sample 1361823153494 24
12:12:33     INFO -  MOZ_EVENT_TRACE sample 1361823153895 123
12:12:35     INFO -  MOZ_EVENT_TRACE sample 1361823155527 26
12:12:39     INFO -  MOZ_EVENT_TRACE sample 1361823159554 22
12:12:39     INFO -  MOZ_EVENT_TRACE sample 1361823159693 39
12:12:39     INFO -  MOZ_EVENT_TRACE sample 1361823159938 34
12:12:40     INFO -  MOZ_EVENT_TRACE sample 1361823160079 30
12:12:40     INFO -  MOZ_EVENT_TRACE sample 1361823160290 111
12:12:42     INFO -  MOZ_EVENT_TRACE sample 1361823162071 124
12:12:55  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/shared/test/browser_browser_basic.js | Test timed out

Comment 9

[Matt Brubeck (:mbrubeck)]

Oops, I thought I had included this in my last Try push but I was mistaken.

Re-landed with the fix for the timeout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71a93fff7311

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out again, this time for turning B2G reftests/crashtests orange. Happened after the first landing too, but our B2G emulator testing situation is so awful that we didn't notice anything was wrong until it was pushed the second time.
https://hg.mozilla.org/integration/mozilla-inbound/rev/67541ee4c5a4

https://tbpl.mozilla.org/php/getParsedLog.php?id=20082290&tree=Mozilla-Inbound for example.

Comment 11

[Matt Brubeck (:mbrubeck)]

For the record (in case the logs expire by the time I get back to this), the B2G reftests failed with:

15:33:29  WARNING -  E/GeckoConsole(  813): [JavaScript Error: "setTimeout is not a function" {file: "chrome://reftest/content/reftest-content.js" line: 125}]

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 712803 [details] [diff] [review]
patch v2

>diff --git a/browser/devtools/shared/Browser.jsm b/toolkit/modules/Timer.jsm

>+function setTimeout(aCallback, aMilliseconds) {
>-this.setTimeout = function setTimeout(callback, delay) {

That'd do it. 

jsloader.reuseGlobal (bug 798491) is only enabled on b2g.

Comment 13

[Matt Brubeck (:mbrubeck)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #12)
> >+function setTimeout(aCallback, aMilliseconds) {
> >-this.setTimeout = function setTimeout(callback, delay) {
> 
> That'd do it. 

Thanks, that did it.  Green on Try:
https://tbpl.mozilla.org/?tree=Try&rev=96cd27113cee

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fa3d903e63f

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5fa3d903e63f

Comment 15

[Matt Brubeck (:mbrubeck)]

I added some basic documentation at:
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Timer.jsm

And filed a follow-up bug 846988 for a minor interop issue.