RESOLVED FIXED 160887
Web Inspector: rapid updates to status icons in Visual Styles sidebar causes flicker
https://bugs.webkit.org/show_bug.cgi?id=160887
Summary Web Inspector: rapid updates to status icons in Visual Styles sidebar causes ...
Matt Baker
Reported 2016-08-15 18:13:23 PDT
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
Attachments
[Video] Icon flicker (239.96 KB, video/mp4)
2016-08-15 18:13 PDT, Matt Baker
no flags
Patch (7.90 KB, patch)
2016-08-16 00:40 PDT, Devin Rousso
no flags
Patch (4.73 KB, patch)
2016-08-16 16:54 PDT, Devin Rousso
no flags
Patch (5.91 KB, patch)
2016-08-17 10:15 PDT, Devin Rousso
no flags
Radar WebKit Bug Importer
Comment 1 2016-08-15 18:13:44 PDT
Devin Rousso
Comment 2 2016-08-16 00:40:48 PDT
Timothy Hatcher
Comment 3 2016-08-16 10:17:30 PDT
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
Comment 4 2016-08-16 16:46:03 PDT
(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
Comment 5 2016-08-16 16:54:56 PDT
Timothy Hatcher
Comment 6 2016-08-17 09:52:26 PDT
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
Comment 7 2016-08-17 10:15:09 PDT
WebKit Commit Bot
Comment 8 2016-08-17 10:50:36 PDT
Comment on attachment 286304 [details] Patch Clearing flags on attachment: 286304 Committed r204562: <http://trac.webkit.org/changeset/204562>
WebKit Commit Bot
Comment 9 2016-08-17 10:50:41 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.