Bug 193737 - Web Inspector: Refactor WI.CSSStyleDeclaration.prototype.update
Summary: Web Inspector: Refactor WI.CSSStyleDeclaration.prototype.update
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-23 14:11 PST by Nikita Vasilyev
Modified: 2019-01-23 17:00 PST (History)
4 users (show)

See Also:


Attachments
Patch (4.04 KB, patch)
2019-01-23 14:29 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (4.18 KB, patch)
2019-01-23 15:29 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Image] Call stack using "soon" (66.76 KB, image/png)
2019-01-23 15:45 PST, Matt Baker
no flags Details
[Image] console.trace with setTimeout (79.87 KB, image/png)
2019-01-23 15:56 PST, Nikita Vasilyev
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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>