[webkit-reviews] review denied: [Bug 180839] Web Inspector: Canvas tab: throttle recording slider updates : [Attachment 329403] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Matt Baker
<mattbaker at apple.com>'s request for review:
Bug 180839: Web Inspector: Canvas tab: throttle recording slider updates
https://bugs.webkit.org/show_bug.cgi?id=180839

Attachment 329403: Patch

https://bugs.webkit.org/attachment.cgi?id=329403&action=review




--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 329403
  --> https://bugs.webkit.org/attachment.cgi?id=329403
Patch

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

> Source/WebInspectorUI/ChangeLog:11
> +	   executes immediately on the leading and trailing edges, and at most

"Immediately on the leading edge" yes, but I don't think it can be "immediate"
on the trailing edge, it is just guaranteed on the trailing edge.

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

Style: `value(delay)` shorthand syntax works fine here.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1496
> +	       if (!this[throttleProxySymbol]) {
> +		   let lastFireTime = NaN;
> +		   let mostRecentArguments = null;
> +
> +		   this[throttleProxySymbol] = new Proxy(this, {

This means that the first call to throttle sets the delay that will be used
forever.

r- since this appears to be different from debounce.

`debounce(delay)` returns a new proxy each time, and that new proxy will apply
`delay` to each subsequent sub access.

Here `throttle(delay)` is cached on the object and forever returned later on.
So `throttle(100); throttle(200)` return the same value (a 100 ms throttle).

I think we should avoid the caching and return a new proxy each time.

While it would be hard to write a non-flakey test for this behavior we should
be able to test manually with something like:

    let startTime;
    let object = {
	test() {
	    console.log("FIRE", (Date.now() - startTime) + "ms");
	}
    };

    setTimeout(() => {
	let throttle1 = object.throttle(20);
	startTime = Date.now();
	throttle1.test();
	setTimeout(() => { throttle1.test() }, 5);
	setTimeout(() => { throttle1.test() }, 12);
	setTimeout(() => { throttle1.test() }, 22);
	setTimeout(() => { throttle1.test() }, 32);
	// Expect:
	// FIRE: 0ms
	// FIRE: 20ms
	// FIRE: 40ms
    }, 0);

    setTimeout(() => {
	let throttle2 = object.throttle(10);
	startTime = Date.now();
	throttle1.test();
	setTimeout(() => { throttle1.test() }, 5);
	setTimeout(() => { throttle1.test() }, 12);
	setTimeout(() => { throttle1.test() }, 22);
	setTimeout(() => { throttle1.test() }, 32);
	// Expect:
	// FIRE: 0ms
	// FIRE: 10ms
	// FIRE: 20ms
	// FIRE: 30ms
	// FIRE: 40ms
    }, 100);

Where FIRE +/- 1ms off. Right now I'd expect the second case to be incorrectly
throttled at 20ms instead of 10ms.

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

Style: `value()`

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:546
> +	  
this.throttle(250).dispatchEventToListeners(WI.RecordingContentView.Event.Recor
dingActionIndexChanged, {index});

Note that this is exactly what I think is broken right now. There are two
throttles on the same object, so I think this throttle is actually 200ms
instead of a 250ms.

That said, I don't think you want the throttle in both places. (they are
competing over throttles for no good reason). Just one place should be enough,
whichever you choose.

> LayoutTests/inspector/unit-tests/throttle.html:14
> +	       const expectedDelay = 20;

We should probably increase 20. Debug builds on the bots often struggle to meet
timeouts that are less than 100ms.

If we increase this to 50ms that might prevent flakiness.

Another possibility is to just count calls and compare timestamps to be greater
than some minimum. And the result should be: (no more than N calls, and less
than N ms). But I think what you have is fine.