Web Inspector: Add debounce to URL inputs in the Visual sidebar

[Devin Rousso]

If I am adding a background-image via url(), it is annoying to see an error every time I type a character until the URL is completed.

[Radar WebKit Bug Importer]

<rdar://problem/24032203>

[Devin Rousso]

Created attachment 268115 [details]
Patch

[Timothy Hatcher]

Comment on attachment 268115 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268115&action=review

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1173
> +function debounce(callback, delay, context) {

{ on newline. context would be better called thisObject.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1174
> +    let timeout = null;

It would be nice if this was stored on the callback with a symbol. Then you can do:

if (callback[TimeoutSymbol])
    return;

And make sure to clear the timeout when it fires. This allows others to debounce the same function from different places and only have it fire once.

This makes me want to put debounce on the Function.prototype, so it is more natural to use.

this._valueInputValueChanged.debounce(250, this)

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1181
> +        let bound = () => {
> +            callback.apply(context, args);
> +        };

Could just inline with the setTimeout.

[Devin Rousso]

Created attachment 268215 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 268215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268215&action=review

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1110
> +WebInspector.FunctionDebounceTimeoutSymbol = Symbol("function-debounce-timeout");

Hmm, normally Utilities.js does not assume the "WebInspector" namespace exists. Currently it only does for localization but that is only if certain functions are called.

You could include this in an anonymous block:

    (function() {
        const debounceSymbol = Symbol("function-debounce-timeout");

        Object.defineProprety(Function.prototype, "debounce", {
            ...
        });
    })();

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1124
> +        let callback = this;
> +        callback[WebInspector.FunctionDebounceTimeoutSymbol] = null;
> +        return function() {
> +            clearTimeout(callback[WebInspector.FunctionDebounceTimeoutSymbol]);
> +            thisObject = thisObject || this;
> +            let args = arguments;
> +            callback[WebInspector.FunctionDebounceTimeoutSymbol] = setTimeout(() => {
> +                callback.apply(thisObject, args);
> +            }, delay);
> +        };

This looks like it is just adding a setTimeout to a bound function. And indeed it does re-implement bind. But we should be able to simplify it.

As written, this may behave in unexpected ways because you are setting the debounce callback on a Foo.prototype.function and not an instance's version of it.

Take this scenario:

    class MyClass {
        constructor() { this._debouncer = this.onchange.debounce(250, this); }
        trigger() { this._debouncer(); }
        onchange() { console.log("change", this); }
    }
    let instance1 = new MyClass;
    let instance2 = new MyClass;
    setInterval(() => { instance1.trigger(); }, 300);
    setTimeout(() => { setInterval(() => { instance2.trigger(); }, 300); }, 100);

My guess is that we would never see a log, despite each instance getting triggered every 300 ms, which is longer than the 250ms bounce time. This is because both `debounce` calls are both bouncing the same function instance (MyClass.prototype.onchange)! The order of operations I would see are:

    0ms - create bouncers
    300ms - clearTimeout, setTimeout for instance1
    400ms - clearTimeout, setTimeout for instance2
    600ms - clearTimeout, setTimeout for instance1
    700ms - clearTimeout, setTimeout for instance2
    ...

Though unlikely, it doesn't seem like what we want. It seems we want to tie the bouncing to a unique instance of the original function, meaning do the bind first! That certainly simplifies things to me:

    value: function(delay, thisObject)
    {
        var callback = this.bind(thisObject);
        return function() {
            clearTimeout(callback[debounceSymbol]);
            callback[debounceSymbol] = setTimeout(callback, delay, ...arguments);
        }
    }

But because of an issue with the spread operator <https://webkit.org/b/152721> I think you need to do what you had with the `args` dance:

    value: function(delay, thisObject)
    {
        let callback = this.bind(thisObject);
        return function() {
            let args = arguments;
            clearTimeout(callback[debounceSymbol]);
            callback[debounceSymbol] = setTimeout(() => { callback.apply(null, args); }, delay);
        }
    }

Now each call to debounce, returns its own instance of the function it was called on.

What do you think?

[Devin Rousso]

Created attachment 268256 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 268256 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268256&action=review

Overall this looks good to me.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1116
> +            let callback = this.bind(thisObject || this);

We don't want the "|| this". Just removing that will mean we match whatever the default behavior of `Function.prototype.bind` is given any arbitrary thisObject, which is what we would want here anyways.

[Devin Rousso]

Created attachment 268266 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 268266 [details]
Patch

Clearing flags on attachment: 268266

Committed r194573: <http://trac.webkit.org/changeset/194573>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.