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

Devin Rousso
Reported 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.
Attachments
Patch (18.17 KB, patch)
2018-11-14 03:02 PST, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (18.01 KB, patch)
2018-11-14 11:37 PST, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Devin Rousso
Comment 1 2018-11-14 03:02:05 PST
Created attachment 354787 [details] Patch
Dean Jackson
Comment 2 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?
Devin Rousso
Comment 3 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.
Devin Rousso
Comment 4 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.
Devin Rousso
Comment 5 2018-11-14 11:37:08 PST
Created attachment 354841 [details] Patch
WebKit Commit Bot
Comment 6 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>
WebKit Commit Bot
Comment 7 2018-11-14 14:03:22 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 8 2018-11-14 14:04:26 PST
<rdar://problem/46074620>
Note You need to log in before you can comment on or make changes to this bug.