Bug 193737 – Web Inspector: Refactor WI.CSSStyleDeclaration.prototype.update

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

.

[Nikita Vasilyev]

Created attachment 359942 [details]
Patch

[Matt Baker]

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);

[Matt Baker]

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.

[Nikita Vasilyev]

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 :(

[Nikita Vasilyev]

Created attachment 359957 [details]
Patch

[Matt Baker]

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.

[Nikita Vasilyev]

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.

[Matt Baker]

(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.

[Matt Baker]

Comment on attachment 359957 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 359957 [details]
Patch

Clearing flags on attachment: 359957

Committed r240368: <https://trac.webkit.org/changeset/240368>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47500145>