[webkit-reviews] review granted: [Bug 180770] Web Inspector: replace HTMLCanvasElement with CanvasRenderingContext for instrumentation logic : [Attachment 329353] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 180770: Web Inspector: replace HTMLCanvasElement with
CanvasRenderingContext for instrumentation logic
https://bugs.webkit.org/show_bug.cgi?id=180770

Attachment 329353: Patch

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




--- Comment #40 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 329353
  --> https://bugs.webkit.org/attachment.cgi?id=329353
Patch

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

r=me

> Source/WebCore/inspector/InspectorCanvas.cpp:96
> +HTMLCanvasElement* InspectorCanvas::getIfHTMLCanvasElement()

I still don't like this name. We normally don't include `get` in a getter name
either.

> Source/WebCore/inspector/InspectorCanvas.cpp:313
> +    auto* canvasElement = getIfHTMLCanvasElement();

Should we have a FIXME here for OffscreenCanvas?

> Source/WebCore/inspector/InspectorCanvas.h:49
> +class OffscreenCanvas;

Not used?

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:153
> +	   errorString = ASCIILiteral("OffscreenCanvas is currently not
supported.");

OffscreenCanvas won't have a node. So probably a better error might be
something like:

    errorString = ASCIILiteral("No node for this canvas");
    errorString = ASCIILiteral("No node for OffscreenCanvas");

Also I know we aren't consistent, but I don't think we have periods with these
errors most of the time.

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:215
> +    auto* canvasElement = inspectorCanvas->getIfHTMLCanvasElement();
> +    if (!canvasElement) {
> +	   errorString = ASCIILiteral("OffscreenCanvas is currently not
supported.");
> +	   return;
> +    }

Would it be possible for an OffscreenCanvas to have client nodes? If not we
should update this error messages.

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:-380
> -    Vector<InspectorCanvas*> inspectorCanvases;
> -    for (auto& inspectorCanvas : m_identifierToInspectorCanvas.values()) {
> -	   if (inspectorCanvas->canvas().document().frame() == &frame)
> -	       inspectorCanvases.append(inspectorCanvas.get());
> -    }
> -
> -    for (auto* inspectorCanvas : inspectorCanvases) {
> -	   String identifier = unbindCanvas(*inspectorCanvas);
> -	   if (m_enabled)
> -	       m_frontendDispatcher->canvasRemoved(identifier);
> -    }

Hmm. So this relies on the GC to eliminate canvases in subframes instead of
immediately eliminating them when the frame goes away? I suppose this is fine
but if I have a long running top frame and short lived sub-frames it might be
annoying if the frontend updates out of sync with the frames coming and going.