Bug 152655 - Web Inspector: Add debounce to URL inputs in the Visual sidebar
Summary: Web Inspector: Add debounce to URL inputs in the Visual sidebar
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
Keywords: InRadar
Depends on:
Reported: 2016-01-02 15:33 PST by Devin Rousso
Modified: 2016-01-04 22:32 PST (History)
8 users (show)

See Also:

Patch (4.09 KB, patch)
2016-01-02 15:36 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (4.35 KB, patch)
2016-01-04 12:05 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (4.26 KB, patch)
2016-01-04 18:24 PST, Devin Rousso
joepeck: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff
Patch (4.01 KB, patch)
2016-01-04 21:33 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2016-01-02 15:33:53 PST
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.
Comment 1 Radar WebKit Bug Importer 2016-01-02 15:34:04 PST
Comment 2 Devin Rousso 2016-01-02 15:36:42 PST
Created attachment 268115 [details]
Comment 3 Timothy Hatcher 2016-01-04 10:39:08 PST
Comment on attachment 268115 [details]

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])

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.
Comment 4 Devin Rousso 2016-01-04 12:05:00 PST
Created attachment 268215 [details]
Comment 5 Joseph Pecoraro 2016-01-04 17:57:04 PST
Comment on attachment 268215 [details]

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() {
            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;
            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?
Comment 6 Devin Rousso 2016-01-04 18:24:59 PST
Created attachment 268256 [details]
Comment 7 Joseph Pecoraro 2016-01-04 18:29:49 PST
Comment on attachment 268256 [details]

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.
Comment 8 Devin Rousso 2016-01-04 21:33:24 PST
Created attachment 268266 [details]
Comment 9 WebKit Commit Bot 2016-01-04 22:32:47 PST
Comment on attachment 268266 [details]

Clearing flags on attachment: 268266

Committed r194573: <http://trac.webkit.org/changeset/194573>
Comment 10 WebKit Commit Bot 2016-01-04 22:32:52 PST
All reviewed patches have been landed.  Closing bug.