Web Inspector: rapid updates to status icons in Visual Styles sidebar causes flicker

[Matt Baker]

Created attachment 286128 [details]
[Video] Icon flicker

Summary:
Rapid updates to status icons in Visual Styles sidebar causes flicker. This needs to be throttled.

Steps to Reproduce:
1. Open Visual Styles sidebar panel
2. Expand Background > Box Shadow
3. Add an item "+"
4. Type a bunch of invalid stuff in the color field.
  => Error icon flickers, sidebar section redraws a lot

[Radar WebKit Bug Importer]

<rdar://problem/27858251>

[Devin Rousso]

Created attachment 286161 [details]
Patch

[Timothy Hatcher]

Comment on attachment 286161 [details]
Patch

There are debounce unit tests, you should add some test cases for this patch for the new behavior. I am not sure why the new behavior is needed either. I worry about the increase of proxy objects if you use different delays on a class.

In practice, I didn't see the need to do this because the timeout is already per function (original[debounceTimeoutSymbol]), and not having a Map and retaining the proxies will make sure the Proxies go away after the debounce fires.

I agree the fix for the bug is to use debounce in the cases you added, but we should split off the debounce utility changes for more discussion.

[Devin Rousso]

(In reply to comment #3)
> There are debounce unit tests, you should add some test cases for this patch
> for the new behavior. I am not sure why the new behavior is needed either. I
> worry about the increase of proxy objects if you use different delays on a
> class.
> 
> In practice, I didn't see the need to do this because the timeout is already
> per function (original[debounceTimeoutSymbol]), and not having a Map and
> retaining the proxies will make sure the Proxies go away after the debounce
> fires.

Ah I didn't consider this.  My only worry was that if we needed to create a debounce proxy for a given timeout for multiple functions then we would duplicate the same proxy multiple times (exactly what Object.prototype.soon did).  Given the issue with GC, it may not be worth it however. 

> I agree the fix for the bug is to use debounce in the cases you added, but
> we should split off the debounce utility changes for more discussion.

I agree.  As it turns out, I also didn't need debounce for a single object that much anyways.

[Devin Rousso]

Created attachment 286233 [details]
Patch

[Timothy Hatcher]

Comment on attachment 286233 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:448
> +    get debouncedValueDidChange()
> +    {
> +        if (!this._debouncedValueDidChange)
> +            this._debouncedValueDidChange = this.debounce(500)._valueDidChange;
> +
> +        return this._debouncedValueDidChange;
> +    }

This shouldn't be needed. The debounce already works if you call it multiple times, since the timeout identifier is stored on the original function.

So doing this will only call _valueDidChange once (it is not scheduled twice):

this.debounce(500)._valueDidChange()
this.debounce(500)._valueDidChange()

[Devin Rousso]

Created attachment 286304 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 286304 [details]
Patch

Clearing flags on attachment: 286304

Committed r204562: <http://trac.webkit.org/changeset/204562>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.