[webkit-reviews] review granted: [Bug 194721] Web Inspector: make debounce Proxy into its own class : [Attachment 362220] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 194721: Web Inspector: make debounce Proxy into its own class
https://bugs.webkit.org/show_bug.cgi?id=194721

Attachment 362220: Patch

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




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

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

r=me

> Source/WebInspectorUI/UserInterface/Base/Debouncer.js:79
> +    delayForFrame()

Up to you but I like these names better:

    delayForTime()
    delayUntilNextFrame()
    delayUntilNextMicrotask()

And there could be a convenience:

    delayUntilNextEventLoop() which is delayForTime(0)

> Source/WebInspectorUI/UserInterface/Base/Throttler.js:75
> +	   if (isNaN(this._lastFireTime)) {
> +	       this._execute();
> +	       return;
> +	   }

If we initialize `this._lastFireTime` to `0` instead of NaN we can eliminate
this case entirely and everything should behave as expected.

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

So this doesn't debounce at all now? Seems fine.

> Source/WebInspectorUI/UserInterface/Views/ContentBrowser.js:95
> +	   this._dispatchCurrentRepresentedObjectsDidChange = new Debouncer(()
=> {

Normally you name members with a suffix such as `fooDebouncer` or
`fooThrottler`, much like we often do for Maps and Sets. I like that pattern.
This one got missed.

   this._displayCurrentRepresentedObjectsDidChangeDebouncer

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:111
> +	   this._generateContentThrottler.fire();

Nice, this even eliminates the need to track `index` because when the throttler
fires it uses `this._index`!

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:47
> +	      
textEditor.addEventListener(WI.TextEditor.Event.ContentDidChange, (event) => {
> +		   contentDidChangeDebouncer.delayForTime(250, event);
> +	       }, this);

This is so much clearer! So much win!

> Source/WebInspectorUI/UserInterface/Views/SpringEditor.js:220
> +	   this._updatePreviewAnimation(event);

Another case where we are removing a debounce? Seems fine

> LayoutTests/inspector/unit-tests/throttler.html:125
> +	       throttler.fire();
> +	       throttler.fire();
> +
> +	       throttler.cancel();

We should assert that this fired once and won't fire the 2nd time. (as you
said, just add a log in the throttler)