Created attachment 457103 [details]
Patch v1.0

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

Comment on attachment 457103 [details]
Patch v1.0

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

r-, while the code in this patch looks fine, I'm a bit concerned with the idea of having Web Inspector keep something alive that would otherwise not be reachable in any way.  Yes, the Console does keep things alive, but it's still possible to reach those things via something like `$42`, whereas in order for a `<canvas>` to be destroyed it must not be strongly referenced by *anyone*, meaning that there's kinda almost no point in keeping it alive since there'd be no way to do anything with it (e.g. you can't call any `<canvas>` APIs with it because you don't have a reference to the object in the first place).  I fear that developers will either a) be confused as to why their `<canvas>` is being kept alive beyond what they'd expect (e.g. if they explicitly go through the motions of clearing all strong references to it) or b) be confused as to why their `<canvas>` isn't being kept alive when Web Inspector is not open.  It also could interfere with the developer trying to figure out ways of decreasing their memory usage, and I'd hate for them to get frustrated trying to find a way to destroy the `<canvas>` only to realize that they forgot about this checkbox.  We've tried *really* hard to make it so that Web Inspector doesn't affect the page beyond what's minimally necessary in order to support stuff like a debugger (e.g. the Elements Tab doesn't keep `Node` alive if they're removed from the DOM tree), so I'd hate to stray from that path now.  Also (and this is really more of a NIT than anything), the Graphics Tab technically instruments rendering contexts, not `<canvas>`, so if a rendering context hasn't yet been created then the relevant/related `<canvas>` won't appear in the Graphics Tab anyways.

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:252
> +    m_showRemovedCanvases = enabled;

I don't think this is good enough.  We'd probably wanna make sure that everything in `CanvasRenderingContext::instances()` is added to `m_retainedCanvasElements` if enabled, and clear `m_retainedCanvasElements` if not so that if the developer decides that they wanna turn off "Show removed canvases" we don't still keep those `CanvasRenderingContext` alive.

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:379
> +    // In order to support inspecting transient canvases inside nested frames, we should forgo unbinding them unless
> +    // this is a main frame navigation.

This seems scary.  I don't think we want anything to outlive the `Frame` it's related to.

I forgot to mention that this is quite a lot of work on our (Web Inspector) part when it's something super trivial for a developer to do (e.g. `window.KEEP_ME_ALIVE = canvas;`) 😅

(In reply to Devin Rousso from comment #3)
> Comment on attachment 457103 [details]
> Patch v1.0
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457103&action=review
> 
> r-, while the code in this patch looks fine, I'm a bit concerned with the
> idea of having Web Inspector keep something alive that would otherwise not
> be reachable in any way. [...]

After some further consideration, I agree. While it isn't difficult for us to provide this switch, it may be difficult for the underlying implications of flipping the switch to be understood by everyone, particularly the memory cost associated with it combined with (at least personally) a tendency to set things and leave them, which could be confusing later. Requiring a developer to keep a reference to a canvas they wish to inspect seems reasonable, since they would need to do the same thing to interrogate it from the console anyways.

Resolving as WONTFIX.