Bug 156756 – Web Inspector: Make debounce use an ES6 Proxy

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Timothy Hatcher]

We can do cool things with this!

[Radar WebKit Bug Importer]

<rdar://problem/25809771>

[Devin Rousso]

(In reply to comment #0)
> We can do cool things with this!

I did not know that these existed, but damn they are awesome!

[Timothy Hatcher]

Created attachment 276748 [details]
Patch

[Timothy Hatcher]

Comment on attachment 276748 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1246
> +        value: function(delay)

This argument is not needed, I'll remove.

[Timothy Hatcher]

Created attachment 276754 [details]
Patch

[Build Bot]

Comment on attachment 276754 [details]
Patch

Attachment 276754 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1187592

New failing tests:
inspector/unit-tests/debounce.html

[Build Bot]

Created attachment 276759 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Devin Rousso]

Comment on attachment 276754 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1209
> +        get: function()

Wouldn't it be valid to just write "get()" instead of "get: function()"?

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1244
> +    Object.defineProperty(Function.prototype, "cancelDebounce",

Since we have a "cancelDebounce" should there also be an argument that will specify whether the debounced function should immediately be invoked?  Something like:

this._element.addEventListener("blur", this.debounce(250)._foo);
...
const invokeNow = true;
this._foo.cancelDebounce(invokeNow);

I could imagine this being used for something like updating codeMirror changes or handling inspector blur events.

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:95
> +            this[key].addEventListener("input", this.debounce(250)._handleNumberInputInput);

Personally, I feel that calling "this.debounce(250)" on something that isn't a function object looks a bit weird, since debounce is really only useful for functions and not anything else.  Instead, maybe we can change "debounce" to "makeDebouncer" (or just "debouncer")?

[Devin Rousso]

Comment on attachment 276754 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1223
> +            return new Proxy(this, {

Something I just realized, but wouldn't this generate a new Proxy object each time "obj.debounce(delay)" is called?  Would it be worth the effort to store a map via symbol on "this" where the key is the value of "delay" and the value is the actual Proxy object?  I can't think of a situation where this actually happens, but it may occur in the future.

this._element.addEventListener("keypress", this.debounce(250)._foo); // new Proxy for 250
this._element.addEventListener("mousemove", this.debounce(250)._bar); // new Proxy for 250 (unnecessary since we could use the same proxy used for click)

[Build Bot]

Comment on attachment 276754 [details]
Patch

Attachment 276754 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1187680

New failing tests:
inspector/unit-tests/debounce.html

[Build Bot]

Created attachment 276761 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 276754 [details]
Patch

Attachment 276754 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1187844

New failing tests:
inspector/unit-tests/debounce.html

[Build Bot]

Created attachment 276767 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Timothy Hatcher]

Created attachment 276769 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 276769 [details]
Patch

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

> LayoutTests/inspector/unit-tests/debounce.html:135
> +            setTimeout(() => { InspectorTest.pass("Debounced function canceled."); resolve(); }, 200);

Style: I'd still split this out over multiple lines, it would be easier to read!

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1199
> +    // The `debounce` function lets you call any function on an object with a delay
> +    // and if the function keeps getting called, the delay gets reset. Since `debounce`
> +    // returns a Proxy, you can cache it and call multiple functions with the same delay.

It seems like this can be prone to user error. There may be multiple debunkers per object, but they compete over the one function to run after a delay.

So if you have one object, with two things you want to delay, having more then one debouncer delaying work, will result in one function getting called.

This seems more dangerous given `soon` uses `debounce` under the hood.

My understanding is that in this example, only "bar" would get called:

    var proxy = myObject.soon;
    proxy.foo();
    proxy.bar();

Or worse, only "hidePopover" would happen here:

    myObject.soon.updateContent();
    myObject.debounce(200).hidePopover();

Could we have a test that verifies this?

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1219
> +        value: function(delay)

Style: "value(delay)"

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1230
> +                        if (original[debounceTimeoutSymbol])
> +                            clearTimeout(original[debounceTimeoutSymbol]);

This is the worrisome case. Any debouncer on the original object will clear any other denouncer's timeout no matter if it was for a different function.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1246
> +        value: function()

Style: value()

[Joseph Pecoraro]

Comment on attachment 276769 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1230
>> +                            clearTimeout(original[debounceTimeoutSymbol]);
> 
> This is the worrisome case. Any debouncer on the original object will clear any other denouncer's timeout no matter if it was for a different function.

This is wrong, Tim pointed out that this is per-function `original` is the function not the object!

[Timothy Hatcher]

Comment on attachment 276769 [details]
Patch

https://trac.webkit.org/changeset/199789

[Blaze Burg]

Comment on attachment 276769 [details]
Patch

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

> LayoutTests/inspector/unit-tests/debounce.html:8
> +    let suite = InspectorTest.createAsyncSuite("Debouce");

Typo: debounce

> LayoutTests/inspector/unit-tests/debounce.html:14
> +            let startTime = performance.now();

Might think about adding MS suffix to denote the units.

> LayoutTests/inspector/unit-tests/debounce.html:28
> +                    InspectorTest.expectThat(bar === "abc", "Second argument is 'abc'.");

It seems arbitrary to use the most recent arguments instead of the first set of arguments. We might need to change this in the future to be debounceLeading and debounceTrailing or similar. Underscore.js has both of these, distinguished with a bool parameter (yuck).

All that said, I think our imagined use cases today do not take any arguments.

> LayoutTests/inspector/unit-tests/debounce.html:43
> +            InspectorTest.fail("Debounced function called immediately.");

It is good style to also include the condition that was violated as well as the violation (in this case, 'Debounced function should be called asynchronously').

[Blaze Burg]

Comment on attachment 276769 [details]
Patch

It might make more sense to proxy the function being called rather than an object that stores it. This would look like:

this.renderAllTheThings.invokeAfter(300) / this.renderAllTheThings.invokeWithin(300)
or

invokeSoon = this.renderAllTheThings.debouncingProxy(300); invokeSoon(); invokeSoon();

As Joe pointed out, the current design precludes debouncing multiple functions stored as properties of the same object, or having different debounce rates for the same function if they use the same referring object. I think this case is likely if we provide a debounce proxy as an event listener. And, it seems weird to me that the debounce interval must be set for the entire object. The alternative examples are slightly more readable since the verb-named thing can be invoked. In contrast, this.soon.dosomething() doesn't follow English object-verb or subject-verb-object ordering.

Before we move forward with this change, I would like to see a revised patch that at least addresses the one-debounce-per-object footgun that Joe pointed out.

[Timothy Hatcher]

(In reply to comment #19)
> Comment on attachment 276769 [details]
> Patch
> 
> It might make more sense to proxy the function being called rather than an
> object that stores it. This would look like:
> 
> this.renderAllTheThings.invokeAfter(300) /
> this.renderAllTheThings.invokeWithin(300)
> or
> 
> invokeSoon = this.renderAllTheThings.debouncingProxy(300); invokeSoon();
> invokeSoon();
> 
> As Joe pointed out, the current design precludes debouncing multiple
> functions stored as properties of the same object, or having different
> debounce rates for the same function if they use the same referring object.
> I think this case is likely if we provide a debounce proxy as an event
> listener. And, it seems weird to me that the debounce interval must be set
> for the entire object. The alternative examples are slightly more readable
> since the verb-named thing can be invoked. In contrast,
> this.soon.dosomething() doesn't follow English object-verb or
> subject-verb-object ordering.
> 
> Before we move forward with this change, I would like to see a revised patch
> that at least addresses the one-debounce-per-object footgun that Joe pointed
> out.

I already landed this patch. As Joe mentioned in #c16, his reading of the patch was wrong. Each function has it's own stored timeout id, not shared by the proxy. The proxy only defines what the delay should be for the functions called on it.