Bug 193737

Summary: Web Inspector: Refactor WI.CSSStyleDeclaration.prototype.update
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: 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 Flags
Patch
none
Patch
none
[Image] Call stack using "soon"
none
[Image] console.trace with setTimeout none

Description Nikita Vasilyev 2019-01-23 14:11:40 PST
.
Comment 1 Nikita Vasilyev 2019-01-23 14:29:34 PST
Created attachment 359942 [details]
Patch
Comment 2 Matt Baker 2019-01-23 15:01:40 PST
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 3 Matt Baker 2019-01-23 15:02:44 PST
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 4 Nikita Vasilyev 2019-01-23 15:25:17 PST
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 :(
Comment 5 Nikita Vasilyev 2019-01-23 15:29:25 PST
Created attachment 359957 [details]
Patch
Comment 6 Matt Baker 2019-01-23 15:45:49 PST
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.
Comment 7 Nikita Vasilyev 2019-01-23 15:56:41 PST
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.
Comment 8 Matt Baker 2019-01-23 16:31:18 PST
(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 9 Matt Baker 2019-01-23 16:32:46 PST
Comment on attachment 359957 [details]
Patch

r=me
Comment 10 WebKit Commit Bot 2019-01-23 16:59:18 PST
Comment on attachment 359957 [details]
Patch

Clearing flags on attachment: 359957

Committed r240368: <https://trac.webkit.org/changeset/240368>
Comment 11 WebKit Commit Bot 2019-01-23 16:59:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2019-01-23 17:00:31 PST
<rdar://problem/47500145>