We can do cool things with this!
<rdar://problem/25809771>
(In reply to comment #0) > We can do cool things with this! I did not know that these existed, but damn they are awesome!
Created attachment 276748 [details] Patch
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.
Created attachment 276754 [details] Patch
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
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
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")?
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)
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
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
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
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
Created attachment 276769 [details] Patch
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()
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!
Comment on attachment 276769 [details] Patch https://trac.webkit.org/changeset/199789
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').
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.
(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.