775328 - move Lazy.jsm to toolkit

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

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

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

Comment 1

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

Attachment: patch

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

Comment 2

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

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).

Comment 3

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

Attachment: add a cancel() method

Comment 4

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

Attachment: add a cancel() method

Comment 5

[Dave Townsend [:mossop]]

(In reply to :Gavin Sharp (use [e-mail] 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"

Comment 6

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

Attachment: add a cancel() method

(sorry for the spam, I suck at mq)

Comment 7

[David Teller [:Yoric] - still alive but not very active]

(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.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

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

Comment 9

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

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.

Comment 11

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

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

Comment 12

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

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 13

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

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.

Comment 14

[Andres Hernandez [:andreshm]]

(In reply to :Gavin Sharp (use [e-mail] 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?

Comment 15

[David Teller [:Yoric] - still alive but not very active]

(In reply to Andres Hernandez [:andreshm] from comment #14)
> (In reply to :Gavin Sharp (use [e-mail] 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.

Comment 16

[Andres Hernandez [:andreshm]]

Attachment: Patch v2

Updated patch.

Comment 17

[Dave Townsend [:mossop]]

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

Comment 18

[Andres Hernandez [:andreshm]]

(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"?

Comment 19

[Dave Townsend [:mossop]]

(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 20

[Andres Hernandez [:andreshm]]

Attachment: Patch v3

Comment 21

[Dave Townsend [:mossop]]

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 22

[Andres Hernandez [:andreshm]]

Attachment: Patch v4

Comment 23

[Dave Townsend [:mossop]]

Comment on attachment 645045 [details] [diff] [review]
Patch v4

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

Excellent, thanks

Comment 24

[Andres Hernandez [:andreshm]]

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

Comment 25

[Andres Hernandez [:andreshm]]

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

Comment 26

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

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

Comment 27

[Andres Hernandez [:andreshm]]

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

Comment 28

[Andres Hernandez [:andreshm]]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

(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

Comment 30

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/ef2c0cf39715

Comment 31

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 32

[Mike Kaply [:mkaply]]

I this something that should be documented on devmo?

Comment 33

[Dave Townsend [:mossop]]

(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

Comment 34

[Matthew N. [:MattN]]

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.