[webkit-reviews] review denied: [Bug 239009] Web Inspector: Regression(r278785) Graphics Tab: Transient canvases can not be inspected : [Attachment 457103] Patch v1.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Patrick Angle <pangle at apple.com>'s
request for review:
Bug 239009: Web Inspector: Regression(r278785) Graphics Tab: Transient canvases
can not be inspected
https://bugs.webkit.org/show_bug.cgi?id=239009

Attachment 457103: Patch v1.0

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




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 457103
  --> https://bugs.webkit.org/attachment.cgi?id=457103
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.