891439 - Standardize the sheduleSearch/performSearch methods

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Victor Porof [:vporof][:vp]]

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.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: v1

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 3

[Victor Porof [:vporof][:vp]]

(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 :)

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

And by that I meant "an *additional* sync version of the API".

Comment 6

[Victor Porof [:vporof][:vp]]

I'll just remove the sync behavior altogether. My fears in comment #3 are not justified anymore.

Comment 7

[Victor Porof [:vporof][:vp]]

Attachment: dbg-drain.patch

Addressed comments, now having a sync-only version of the API. The methods are named "setNamedTimeout" and "clearNamedTimeout". Carry over r+.

Comment 8

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/c5b199404e8a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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