Closed Bug 840360 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #712717 - Flags: superreview?(gavin.sharp)
Attachment #712717 - Flags: review?(jwalker)
Blocks: 622224
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.
Attached patch patch v2Splinter Review
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Attachment #712717 - Attachment is obsolete: true
Attachment #712717 - Flags: superreview?(gavin.sharp)
Attachment #712717 - Flags: review?(jwalker)
Attachment #712803 - Flags: superreview?(gavin.sharp)
Attachment #712803 - Flags: review?(jwalker)
Attachment #712803 - Flags: review?(jones.chris.g)
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.
Attachment #712803 - Flags: review?(jwalker) → review+
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]
Attachment #712803 - Flags: superreview?(gavin.sharp) → superreview+
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.
Attachment #712803 - Flags: review?(jones.chris.g) → review+
(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
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
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
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.
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 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.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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
https://hg.mozilla.org/mozilla-central/rev/5fa3d903e63f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 846988
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.
Flags: in-testsuite+
Blocks: 855015
You need to log in before you can comment on or make changes to this bug.