Bug 190525

Summary: Web Inspector: Canvas: previews aren't removed when the parent view is hidden
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, inspector-bugzilla-changes, joepeck, mattbaker, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 175485    
Attachments:
Description Flags
Patch
none
Patch none

Devin Rousso
Reported 2018-10-12 09:55:32 PDT
* STEPS TO REPRODUCE: 1. Inspect <https://devinrousso.com/demo/WebKit/canvas.html> 2. Go to the Canvas tab 3. Refresh the page 4. Go to any other tab 5. Refresh the page 6. Go back to the Canvas tab => A Canvas preview with "No Preview Available" is shown for each canvas that was on the page before it was refreshed
Attachments
Patch (1.95 KB, patch)
2018-10-12 16:17 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (2.79 KB, patch)
2018-10-12 16:35 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Devin Rousso
Comment 1 2018-10-12 16:17:37 PDT
Created attachment 352231 [details] Patch
Matt Baker
Comment 2 2018-10-12 16:22:56 PDT
Comment on attachment 352231 [details] Patch r=me
Joseph Pecoraro
Comment 3 2018-10-12 16:23:57 PDT
Comment on attachment 352231 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352231&action=review > Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:138 > for (let canvas of this._canvasCollection) { > - if (!canvases.has(canvas)) > + if (!WI.canvasManager.canvases.includes(canvas)) > this._removeCanvas(canvas); > } > > - for (let canvas of canvases) { > + for (let canvas of WI.canvasManager.canvases) { > if (!this._canvasCollection.has(canvas)) > this._addCanvas(canvas); > } Given that `WI.canvasManager.canvases` is a non-simple getter we should only run it once: let canvases = WI.canvasManager.canvases; And use this everywhere instead of `WI.canvasManager.canvases` everywhere.
Devin Rousso
Comment 4 2018-10-12 16:35:46 PDT
Created attachment 352236 [details] Patch
Matt Baker
Comment 5 2018-10-12 16:44:37 PDT
(In reply to Joseph Pecoraro from comment #3) > Comment on attachment 352231 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=352231&action=review > > > Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:138 > > for (let canvas of this._canvasCollection) { > > - if (!canvases.has(canvas)) > > + if (!WI.canvasManager.canvases.includes(canvas)) > > this._removeCanvas(canvas); > > } > > > > - for (let canvas of canvases) { > > + for (let canvas of WI.canvasManager.canvases) { > > if (!this._canvasCollection.has(canvas)) > > this._addCanvas(canvas); > > } > > Given that `WI.canvasManager.canvases` is a non-simple getter we should only > run it once: > > let canvases = WI.canvasManager.canvases; > > And use this everywhere instead of `WI.canvasManager.canvases` everywhere. Good eye! I didn't think to check the getter.
WebKit Commit Bot
Comment 6 2018-10-12 16:52:18 PDT
Comment on attachment 352236 [details] Patch Clearing flags on attachment: 352236 Committed r237090: <https://trac.webkit.org/changeset/237090>
WebKit Commit Bot
Comment 7 2018-10-12 16:52:19 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 8 2018-10-12 16:53:31 PDT
<rdar://problem/45241822>
Note You need to log in before you can comment on or make changes to this bug.