Closed
Bug 770920
Opened 12 years ago
Closed 12 years ago
Create a module BufferingUtils.jsm
Categories
(Toolkit :: General, enhancement)
Toolkit
General
Tracking
()
RESOLVED
DUPLICATE
of bug 775328
People
(Reporter: Yoric, Assigned: andreshm)
Details
(Whiteboard: [mentor=Yoric][lang=js][good first bug])
Attachments
(1 file)
(deleted),
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 2•12 years ago
|
||
I will be away for a few weeks, so not very available for mentoring. I will be available for reviewing, though.
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #642802 -
Flags: review?(dteller)
Comment 4•12 years ago
|
||
What a coincidence. :) I suspect you'll just want to build on top of bug 775328?
Depends on: 775328
Reporter | ||
Comment 5•12 years ago
|
||
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.
Reporter | ||
Comment 6•12 years ago
|
||
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+
Comment 7•12 years ago
|
||
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!
You need to log in
before you can comment on or make changes to this bug.
Description
•