I'd like to use it for the social patch in bug 774003, so I figured I'd move it to toolkit.

This dumps it in toolkit/content, and adds a test.

CCing a bunch of people who might know of other potential users (or who might want to bikeshed the name, "Lazy" is kind of generic).

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #2) > CCing a bunch of people who might know of other potential users (or who > might want to bikeshed the name, "Lazy" is kind of generic). Not totally against Lazy, but wondering about alternatives like "Deferred" or "Coalesced"

(sorry for the spam, I suck at mq)

(In reply to Dave Townsend (:Mossop) from comment #5) > Not totally against Lazy, but wondering about alternatives like "Deferred" > or "Coalesced" I vote for Deferred.

FYI: except for cosmetic changes, this is the same as bug 770920.

Oops, wasn't aware of that bug! They really do seem the same, apart from naming, but this patch has tests and extends the API somewhat.

Another potential API addition: isPending property, to check whether the timer is currently running.

I merged the patches as suggested by Gavin and used 'Buffering' for the name, but let me know if you prefer to use 'Lazy', 'Deferred' or other one. Also, I added the isPending property with a test.

Comment on attachment 644064 [details] [diff] [review] Patch v1 "Buffering" on its own also seems a bit odd. "DeferredTask", maybe? Why change the order of the arguments? (code, timeout) matches setTimeout, and also leaves open the possibility of making the timeout optional (by introducing a default value). I noticed a typo in testIsPending: the last two lines check lazy1.isPending twice, the second is meant to be lazy2.isPending. The alphabetization of the test list in the Makefile doesn't serve any useful purpose, please omit that :) f=me, but someone else should review this since I partially wrote this, and I wanted other's opinions anyways.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #13) > Comment on attachment 644064 [details] [diff] [review] > Patch v1 > > "Buffering" on its own also seems a bit odd. "DeferredTask", maybe? Sure, I'll apply the change. > Why change the order of the arguments? (code, timeout) matches setTimeout, > and also leaves open the possibility of making the timeout optional (by > introducing a default value). It was suggested by David in Bug 770920. > f=me, but someone else should review this since I partially wrote this, and > I wanted other's opinions anyways. Who should I ask review for this one?

(In reply to Andres Hernandez [:andreshm] from comment #14) > (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment > #13) > > Comment on attachment 644064 [details] [diff] [review] > > Patch v1 > > > > "Buffering" on its own also seems a bit odd. "DeferredTask", maybe? > > Sure, I'll apply the change. > > > Why change the order of the arguments? (code, timeout) matches setTimeout, > > and also leaves open the possibility of making the timeout optional (by > > introducing a default value). > > It was suggested by David in Bug 770920. Indeed, this is my fault. With the exception of setTimeout, just about every JS function I have seen that takes a callback puts this callback as last argument. This sounds like a good convention to me. > > > f=me, but someone else should review this since I partially wrote this, and > > I wanted other's opinions anyways. > > Who should I ask review for this one? Since Gavin asked Mossop (Dave Townsend) for review, I would say this is the right person.

Updated patch.

Comment on attachment 644378 [details] [diff] [review] Patch v2 Review of attachment 644378 [details] [diff] [review]: ----------------------------------------------------------------- A couple of changes to make here, mostly good though. I'm a little nervous about using timing in tests, but there isn't much of a way around it. Hopefully the times are long enough that it won't cause any problems. Not totally sold on "go" as the method name here (it feels like an immediate thing) anyone have alternate ideas? Maybe "trigger" is about the only one that is springing to my mind right now. Can you add a test that does something like this: Creates TaskA with a delay of 100ms Creates TaskB with a delay of 50ms Starts both In TaskB start TaskA again and trigger a new TaskC of 75ms In TaskC make sure TaskA hasn't run yet. Check TaskA eventually runs. ::: toolkit/content/DeferredTask.jsm @@ +7,5 @@ > +const Cc = Components.classes; > +const Ci = Components.interfaces; > + > +/** > + * A delayed treatment that may be delayed even further. "A task that will run after a delay. Multiple attempts to run the same task before the delay will be coalesced" @@ +27,5 @@ > + aCallback(); > + this._timer = null; > + }.bind(this); > + this._delay = aDelay; > + this._timer = null; It's already null, no need to do it again I think @@ +39,5 @@ > + /* Timer */ > + _timer: null, > + > + /** > + * Check the current treatment state. I don't really think the word "treatment" fits. Can you change it to "task" throughout? ::: toolkit/content/tests/browser/browser_DeferredTask.js @@ +34,5 @@ > + > + deferredTask.go(); > + deferredTask.go(); > + deferredTask.go(); > + }, Could you put newlines between these functions for readability please. @@ +36,5 @@ > + deferredTask.go(); > + deferredTask.go(); > + }, > + function testOrdering(next) { > + // Test that a deferred task runs only once Correct this comment @@ +94,5 @@ > + function testIsPending(next) { > + let deferredTask1, deferredTask2; > + let deferredTask1 = new DeferredTask(function () { > + info("Deferred task 1 running"); > + is(deferredTask2.isPending, true, "Deferred task 2 should be pending"); Test the value of deferredTask1.isPending here too @@ +98,5 @@ > + is(deferredTask2.isPending, true, "Deferred task 2 should be pending"); > + }, 100); > + let deferredTask2 = new DeferredTask(function () { > + info("Deferred task 2 running"); > + is(deferredTask1.isPending, false, "Deferred task 1 shouldn't be pending"); Test the value of deferredTask2.isPending here too

(In reply to Dave Townsend (:Mossop) from comment #17) > Comment on attachment 644378 [details] [diff] [review] > Patch v2 > > Review of attachment 644378 [details] [diff] [review]: > ----------------------------------------------------------------- > > Not totally sold on "go" as the method name here (it feels like an immediate > thing) anyone have alternate ideas? Maybe "trigger" is about the only one > that is springing to my mind right now. What about "start"?

(In reply to Andres Hernandez [:andreshm] from comment #18) > (In reply to Dave Townsend (:Mossop) from comment #17) > > Comment on attachment 644378 [details] [diff] [review] > > Patch v2 > > > > Review of attachment 644378 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Not totally sold on "go" as the method name here (it feels like an immediate > > thing) anyone have alternate ideas? Maybe "trigger" is about the only one > > that is springing to my mind right now. > > What about "start"? Yeah that sounds ok to me

Comment on attachment 644467 [details] [diff] [review] Patch v3 Review of attachment 644467 [details] [diff] [review]: ----------------------------------------------------------------- This looks great, but I did spot an additional problem. Looks like it was in the original implementation that we're moving but might as well fix it while we're here. Should be a quick r+ after that. ::: toolkit/content/DeferredTask.jsm @@ +23,5 @@ > + * @param aCallback The code to execute after the delay. > + */ > +function DeferredTask(aCallback, aDelay) { > + this._callback = function onCallback() { > + aCallback(); Wrap the callback in a try...catch block and log any exception to the error console with Components.utils.reportError please. ::: toolkit/content/tests/browser/browser_DeferredTask.js @@ +101,5 @@ > + // Test the pending property. > + let deferredTask1, deferredTask2; > + let deferredTask1 = new DeferredTask(function () { > + info("Deferred task 1 running"); > + is(deferredTask1.isPending, true, "Deferred task 1 should be running"); I don't think this is the correct behaviour, if the task is running then it shouldn't be pending. I think that reveals another issue: let task = new DeferredTask(function() { task.start(); }, 100); tast.start(); Here a task attempts to restart itself, but I think it will fail. While the callback is executing _timer is not null, so it'll just set the delay property, but since the timer has already fired that'll throw an exception all the way back into _callback so _timer is never set to null and the DeferredTask is unusable. I think just setting _timer to null before calling the callback is enough to fix both these things, please add a test for the above case though. @@ +124,5 @@ > + // Test restarting a task. > + let deferredTask1, deferredTask2, deferredTask3; > + let deferredTask1 = new DeferredTask(function () { > + info("Deferred task 1 running"); > + is(deferredTask1.isPending, true, "Deferred task 1 should be running"); Add a test that deferredTask3 is not pending here.

Comment on attachment 645045 [details] [diff] [review] Patch v4 Review of attachment 645045 [details] [diff] [review]: ----------------------------------------------------------------- Excellent, thanks

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=2d8a3a10c4c6

Dave please check try results: https://tbpl.mozilla.org/?tree=Try&rev=2d8a3a10c4c6

The try results won't load anymore because it's too far in the past. Were there any test failures?

I'm not really sure, so I pushed it to try again: https://tbpl.mozilla.org/?tree=Try&rev=be0c3028677f

Try finished with just one orange. See: https://tbpl.mozilla.org/?tree=Try&rev=be0c3028677f

(In reply to Andres Hernandez [:andreshm] from comment #28) > https://tbpl.mozilla.org/?tree=Try&rev=be0c3028677f Green on Try. Thanks for the patch, Andres! https://hg.mozilla.org/integration/mozilla-inbound/rev/ef2c0cf39715

Oh dear, oh dear! The test in this patch is failing intermittently: https://tbpl.mozilla.org/php/getParsedLog.php?id=14894336&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=14891229&tree=Mozilla-Inbound

I this something that should be documented on devmo?

(In reply to Michael Kaply (mkaply) from comment #32) > I this something that should be documented on devmo? Absolutely, though I wonder if it should be backed out because of the intermittent failure

I ported the code comments to MDN: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/DeferredTask.jsm Feel free to improve the page.