Closed Bug 770920 Opened 12 years ago Closed 12 years ago

Create a module BufferingUtils.jsm

Categories

(Toolkit :: General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 775328

People

(Reporter: Yoric, Assigned: andreshm)

Details

(Whiteboard: [mentor=Yoric][lang=js][good first bug])

Attachments

(1 file)

SearchService.jsm contains a nice little class called Lazy that serves for buffering (delaying? lazifying? not sure of the vocabulary here) calls to a function. I believe that we should extract this code and make it available in toolkit/. The behavior of this as follows: - create a Lazy wrapping a call; - whenever you are ready to launch the call, invoke |your_lazy.go()|; - this starts a timer for |DELAY| ms; - if no other call to |go()| takes place within |DELAY| ms, the function is called; - on the other hand, if |go()| is called again within |DELAY| ms, the timer is reset. In nsSearchService, we use it to avoid accessing the disk too often with successive updates to the same data structure. If this is made accessible to the rest of the code, I would personally use it with OS.File for buffering of writes. I can also imagine uses for UX, for instance in bug 743069.
I can take a look at this.
Assignee: nobody → andres
I will be away for a few weeks, so not very available for mentoring. I will be available for reviewing, though.
Attached patch Patch v1 (deleted) — Splinter Review
Attachment #642802 - Flags: review?(dteller)
What a coincidence. :) I suspect you'll just want to build on top of bug 775328?
Depends on: 775328
Seems that there is a collision here. The same bug was just opened by Gavin under a different name and with a patch: bug 775328.
Comment on attachment 642802 [details] [diff] [review] Patch v1 Review of attachment 642802 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, with a few minor nitpicks. Now, the collision between patches is a bit annoying. r+ from me with these nitpicks fixed and a test suite. ::: toolkit/content/BufferingUtils.jsm @@ +19,5 @@ > + * during this period, > + * 1/ only the final version of the data is actually written; > + * 2/ a further grace delay is added to take into account other changes. > + */ > +function BufferingUtils(aCallback, aDelay) { More documentation about aCallback and aDelay (e.g. which time unit?) might be useful. Also, for a public API, habit has it that delay comes first and callback second. @@ +24,5 @@ > + this._init(aCallback, aDelay); > +} > + > +BufferingUtils.prototype = Object.freeze({ > + /* Timer */ What is the point of putting all of this in the prototype? @@ +38,5 @@ > + * @param aDelay An optional delay, in milliseconds. > + */ > + _init: function bufferingutils_init(aCode, aDelay) { > + this._callback = (function() { > + aCallback(); Might be a good idea to give a name to this anonymous function.
Attachment #642802 - Flags: review?(dteller) → review+
Oops, I hadn't seen this bug before filing bug 775328. I think this patch's naming is closer to what we want (though I think "Utils" is an odd thing to throw in there - it makes the module sound like it contains many utility functions, when it's really only defining a single type of helper object) and the comments look better, but the patch in bug 775328 has an API addition (cancel) and some tests. Can we consolidate work in that bug and merge the patches? I'd be happy to let you take over, Andres!
Status: NEW → RESOLVED
Closed: 12 years ago
No longer depends on: 775328
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: