Bug 191628

Summary: Web Inspector: Canvas: send a call stack with each action instead of an array of call frames
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 173807    
Attachments:
Description Flags
Patch
none
Patch none

Description Devin Rousso 2018-11-14 02:36:12 PST
Sending an array of the same frames over and over for actions repeated multiple times in the same recording is wasteful.  We should be de-duplicating both per-frame and per-stack.
Comment 1 Devin Rousso 2018-11-14 03:02:05 PST
Created attachment 354787 [details]
Patch
Comment 2 Dean Jackson 2018-11-14 10:09:03 PST
Comment on attachment 354787 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=354787&action=review

> Source/WebInspectorUI/UserInterface/Models/Recording.js:322
> +                case WI.Recording.Swizzle.CallStack:
> +                    var array = await this.swizzle(data, WI.Recording.Swizzle.Array);
> +                    var callFrames = [];
> +                    for (let item of array)
> +                        callFrames.push(await this.swizzle(item, WI.Recording.Swizzle.CallFrame));
> +                    this._swizzle[index][type] = callFrames;
> +                    break;

Why var instead of const/let?

Also, what about this?

const callFrames = array.map(item => { return await this.swizzle(item, WI.Recording.Swizzle.CallFrame); });

Hmmm... I guess that doesn't work because the arrow needs to be async. Can you do that?
Comment 3 Devin Rousso 2018-11-14 10:14:47 PST
Comment on attachment 354787 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=354787&action=review

>> Source/WebInspectorUI/UserInterface/Models/Recording.js:322
>> +                    break;
> 
> Why var instead of const/let?
> 
> Also, what about this?
> 
> const callFrames = array.map(item => { return await this.swizzle(item, WI.Recording.Swizzle.CallFrame); });
> 
> Hmmm... I guess that doesn't work because the arrow needs to be async. Can you do that?

`let` is block scoped, and a switch-case only has one block
<https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let#Redeclarations>

I'd have to do something like:

    this._swizzle[index][type] = await Promise.all(array.map((item) => this.swizzle(item, WI.Recording.Swizzle.CallFrame));

That reads a bit worse (more confusing) IMO.
Comment 4 Devin Rousso 2018-11-14 11:37:03 PST
Comment on attachment 354787 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=354787&action=review

>>> Source/WebInspectorUI/UserInterface/Models/Recording.js:322
>>> +                    break;
>> 
>> Why var instead of const/let?
>> 
>> Also, what about this?
>> 
>> const callFrames = array.map(item => { return await this.swizzle(item, WI.Recording.Swizzle.CallFrame); });
>> 
>> Hmmm... I guess that doesn't work because the arrow needs to be async. Can you do that?
> 
> `let` is block scoped, and a switch-case only has one block
> <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let#Redeclarations>
> 
> I'd have to do something like:
> 
>     this._swizzle[index][type] = await Promise.all(array.map((item) => this.swizzle(item, WI.Recording.Swizzle.CallFrame));
> 
> That reads a bit worse (more confusing) IMO.

Actually, we should do this, as it will allow more things to be enqueued at once.
Comment 5 Devin Rousso 2018-11-14 11:37:08 PST
Created attachment 354841 [details]
Patch
Comment 6 WebKit Commit Bot 2018-11-14 14:03:21 PST
Comment on attachment 354841 [details]
Patch

Clearing flags on attachment: 354841

Committed r238199: <https://trac.webkit.org/changeset/238199>
Comment 7 WebKit Commit Bot 2018-11-14 14:03:22 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2018-11-14 14:04:26 PST
<rdar://problem/46074620>