[webkit-reviews] review denied: [Bug 180839] Web Inspector: Canvas tab: throttle recording slider updates : [Attachment 330741] 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 330741: Patch

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




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

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

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1485
> +    const throttleTimeoutSymbol = Symbol("throttle-timeout");

I really dislike how we've implemented both debounce and throttle to have this
shared symbol. So two throttles on the same object end up smashing over each
other. But that is not specific to your patch that is preexisting poor design.

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:118
>	       if (this.representedObject.type === WI.Recording.Type.Canvas2D)
> -		   this._generateContentCanvas2D(index, actions, options);
> +		   this.throttle(200)._generateContentCanvas2D(index, actions,
options);
>	       else if (this.representedObject.type ===
WI.Recording.Type.CanvasWebGL)
> -		   this._generateContentCanvasWebGL(index, actions, options);
> +		   this.throttle(200)._generateContentCanvasWebGL(index,
actions, options);

r- This code doesn't appear to throttle anything.

Throttling only happens when you have a throttle proxy and invoke actions on
that proxy many times:

    let throttler = this.thottle(200)
    throttle.action();
    throttle.action();
    throttle.action();

However this code creates a new throttle every time and makes 1 call on it,
which will perform immediately every time.