WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Patch
(7.90 KB, patch)
2016-08-16 00:40 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Patch
(4.73 KB, patch)
2016-08-16 16:54 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Patch
(5.91 KB, patch)
2016-08-17 10:15 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2016-08-15 18:13:44 PDT
<
rdar://problem/27858251
>
Devin Rousso
Comment 2
2016-08-16 00:40:48 PDT
Created
attachment 286161
[details]
Patch
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
Created
attachment 286233
[details]
Patch
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
Created
attachment 286304
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug