Closed Bug 891439 Opened 8 years ago Closed 8 years ago

Standardize the sheduleSearch/performSearch methods

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: vporof, Assigned: vporof)

References

Details

Attachments

(1 file, 1 obsolete file)

There's an pattern occurring over and over again in which
1. I want to do something
2. But I'm depending on events that fire quickly and repeatedly
3. And I only want to do it once after things have settled down

This is mostly proeminent with search operations, but it's also been, naturally, occurring in the network monitor as well. Fortunately, there's a nice way to DRY all of this up.

This work continues the endeavor in bug 886848. Most of the code paths are already exercised by existing tests, but I'll make sure everything is accounted for in bug 876277, which I plan to work on next.
Depends on: 886848
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #772739 - Flags: review?(past)
Blocks: 876277
Comment on attachment 772739 [details] [diff] [review]
v1

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

I have a few comments on the API, but I do agree with the change. It simplifies the code a lot!

::: browser/devtools/shared/widgets/ViewHelpers.jsm
@@ +152,5 @@
> +   *        If it's less than or equal to 0, a callback is invoked immediately.
> +   * @param function aCallback
> +   *        Invoked when no more events are fired after the specified time.
> +   */
> +  drainCalls: (() => {

I'm not so sure about the method name. "drainCalls" does not describe the main function of this method, which is to schedule a callback for a future invocation, but instead focuses on one of its traits, that it overrides previously-set-but-not-fired callbacks with the same identifier.

I'd like the method comment to elaborate more on the behavior of the method. As for its name, how about something like scheduleCall, setTimeout or setNamedTimeout?

@@ +162,5 @@
> +      if (!aCallback) {
> +        return;
> +      }
> +      if (aWait <= 0) {
> +        return void aCallback();

I don't think it's a good idea to switch from an async to a sync behavior depending on the value of one argument. Maybe I'm brainwashed by dherman's Essential JavaScript, but I feel that the potential for confusion for consumers of this API is significant ("will the callback or the code following this call get executed first?").

I'd rather just use a "setTimeout(aCallback, 0)" here instead.
Attachment #772739 - Flags: review?(past) → review+
Priority: -- → P2
(In reply to Panos Astithas [:past] from comment #2)
> Comment on attachment 772739 [details] [diff] [review]
> 
> I'd like the method comment to elaborate more on the behavior of the method.
> As for its name, how about something like scheduleCall, setTimeout or
> setNamedTimeout?
> 

setNamedTimeout sounds great, I'll use that. With your help, off-by-one errors will be the only remaining hard thing in programming :)

> I don't think it's a good idea to switch from an async to a sync behavior
> depending on the value of one argument. Maybe I'm brainwashed by dherman's
> Essential JavaScript, but I feel that the potential for confusion for
> consumers of this API is significant ("will the callback or the code
> following this call get executed first?").
> 
> I'd rather just use a "setTimeout(aCallback, 0)" here instead.

I thought about this a lot as well. Normally, I would have also went with "timeouts always meaning deferred timeouts", but the way things work right now required an easy way of allowing those callbacks to fire synchronously (mostly because of tests methinks, but maybe some other reasons I can't necessarily accurately pinpoint). If I were to use a setTimeout(callback, 0) in such cases, then I would also have to add a new method everywhere that fires the respective action synchronously.

How strongly do you feel about this? I'm 50-50 :)
(In reply to Victor Porof [:vp] from comment #3)
> How strongly do you feel about this? I'm 50-50 :)

Your argument doesn't sound convincing enough to forgo good API design. A sync version sounds fine if some call sites really need it and it should help crystallize the API requirements.
And by that I meant "an *additional* sync version of the API".
I'll just remove the sync behavior altogether. My fears in comment #3 are not justified anymore.
Attached patch dbg-drain.patchSplinter Review
Addressed comments, now having a sync-only version of the API. The methods are named "setNamedTimeout" and "clearNamedTimeout". Carry over r+.
Attachment #772739 - Attachment is obsolete: true
Attachment #789428 - Flags: review+
Blocks: 881219
Blocks: 894311
Blocks: 876633
https://hg.mozilla.org/mozilla-central/rev/c5b199404e8a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.