Summary: | Web Inspector: Refactor WI.CSSStyleDeclaration.prototype.update | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||||
Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, inspector-bugzilla-changes, mattbaker, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Nikita Vasilyev
2019-01-23 14:11:40 PST
Created attachment 359942 [details]
Patch
Comment on attachment 359942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359942&action=review > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:143 > + for (let i = 0; i < oldProperties.length; ++i) { You can use for...of since the index isn't used. > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:161 > + for (let i = 0; i < this._enabledProperties.length; ++i) { This loop can be eliminated; it's only purpose was to calculate addedProperties. > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:-177 > - return; Is this check still important? Did it prevent the firing of a PropertiesChanged event when nothing was actually added or removed? > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:172 > + this.dispatchEventToListeners(WI.CSSStyleDeclaration.Event.PropertiesChanged); While you're here, all of this can be replaced by using the soon (debounce) method: this.soon.dispatchEventToListeners(WI.CSSStyleDeclaration.Event.PropertiesChanged); Comment on attachment 359942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359942&action=review > Source/WebInspectorUI/ChangeLog:8 > + Don't pass addedProperties and removedProperties to WI.CSSStyleDeclaration.Event.PropertiesChanged I'd simplify this to: 'Remove unused event data from the WI.CSSStyleDeclaration.Event.PropertiesChanged event.' > Source/WebInspectorUI/ChangeLog:11 > + Use `let` instead of `var`. This comment isn't needed. Comment on attachment 359942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359942&action=review >> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:143 >> + for (let i = 0; i < oldProperties.length; ++i) { > > You can use for...of since the index isn't used. Nice! >> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:161 >> + for (let i = 0; i < this._enabledProperties.length; ++i) { > > This loop can be eliminated; it's only purpose was to calculate addedProperties. Oh, right! >> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:-177 >> - return; > > Is this check still important? Did it prevent the firing of a PropertiesChanged event when nothing was actually added or removed? This doesn't quite make sense anymore. If the text is unchanged, we should never fire PropertiesChanged. >> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:172 >> + this.dispatchEventToListeners(WI.CSSStyleDeclaration.Event.PropertiesChanged); > > While you're here, all of this can be replaced by using the soon (debounce) method: > > this.soon.dispatchEventToListeners(WI.CSSStyleDeclaration.Event.PropertiesChanged); Unfortunately, I find this debug unfriendly. Error traces of anything listening for PropertiesChanged wouldn't have any mention of CSSStyleDeclaration :( Created attachment 359957 [details]
Patch
Created attachment 359964 [details]
[Image] Call stack using "soon"
Debouncing the event will alter the call stack, but the frame from CSSStyleDeclaration.js is visible (see attached). Debounce (soon) uses setTimeout, so its really no different.
Using console.trace won't show the async call frames, but that is no different when using a plain setTimeout.
I'll leave it up to you.
Created attachment 359966 [details] [Image] console.trace with setTimeout (In reply to Matt Baker from comment #6) > Created attachment 359964 [details] > [Image] Call stack using "soon" > > Debouncing the event will alter the call stack, but the frame from > CSSStyleDeclaration.js is visible (see attached). Debounce (soon) uses > setTimeout, so its really no different. Yes, it's fine in the debugger. However, we still don't stitch async stack traces in the console. > > Using console.trace won't show the async call frames, but that is no > different when using a plain setTimeout. It is different! It mentions CSSStyleDeclaration.js. > > I'll leave it up to you. I don't feel strongly about it, but I'd prefer to keep setTimeout for now. (In reply to Nikita Vasilyev from comment #7) > Created attachment 359966 [details] > [Image] console.trace with setTimeout > > (In reply to Matt Baker from comment #6) > > Created attachment 359964 [details] > > [Image] Call stack using "soon" > > > > Debouncing the event will alter the call stack, but the frame from > > CSSStyleDeclaration.js is visible (see attached). Debounce (soon) uses > > setTimeout, so its really no different. > > Yes, it's fine in the debugger. However, we still don't stitch async stack > traces in the console. > > > > > Using console.trace won't show the async call frames, but that is no > > different when using a plain setTimeout. > > It is different! It mentions CSSStyleDeclaration.js. Oh right. I was forgetting that the timer callback will show up in CSSStyleDeclaration. Comment on attachment 359957 [details]
Patch
r=me
Comment on attachment 359957 [details] Patch Clearing flags on attachment: 359957 Committed r240368: <https://trac.webkit.org/changeset/240368> All reviewed patches have been landed. Closing bug. |