Bug 191628 - Web Inspector: Canvas: send a call stack with each action instead of an array of call frames
Summary: Web Inspector: Canvas: send a call stack with each action instead of an array...
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: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: WebInspectorCanvasRecording
  Show dependency treegraph
 
Reported: 2018-11-14 02:36 PST by Devin Rousso
Modified: 2018-11-14 14:04 PST (History)
7 users (show)

See Also:


Attachments
Patch (18.17 KB, patch)
2018-11-14 03:02 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (18.01 KB, patch)
2018-11-14 11:37 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

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