[webkit-reviews] review granted: [Bug 172623] Web Inspector: Instrument 2D/WebGL canvas contexts in the backend : [Attachment 313041] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jun 15 18:56:51 PDT 2017


Devin Rousso <drousso at apple.com> has granted Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 172623: Web Inspector: Instrument 2D/WebGL canvas contexts in the backend
https://bugs.webkit.org/show_bug.cgi?id=172623

Attachment 313041: Patch

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




--- Comment #49 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 313041
  --> https://bugs.webkit.org/attachment.cgi?id=313041
Patch

r=me

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

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:111
> +    if (!m_enabled) {
> +	   m_canvasEntries.clear();
> +	   return;
> +    }
> +
> +    for (auto* canvasElement : canvasesForFrame) {
> +	   auto canvasEntry = m_canvasEntries.take(canvasElement);
> +	   m_frontendDispatcher->canvasRemoved(canvasEntry.identifier);
> +    }

I don't think we want to do this.  We should only be clearing the canvases for
the frame that navigated:

    for (auto* canvasElement : canvasesForFrame) {
	auto canvasEntry = m_canvasEntries.take(canvasElement);
	if (m_enabled)
	    m_frontendDispatcher->canvasRemoved(canvasEntry.identifier);
    }

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:132
> +	   newCanvasEntry.cssCanvasName =
m_canvasToCSSCanvasId.get(&canvasElement);
> +	   m_canvasToCSSCanvasId.remove(&canvasElement);

I think you can just use `take` here like above:

    if (m_canvasToCSSCanvasId.contains(&canvasElement))
	newCanvasEntry.cssCanvasName =
m_canvasToCSSCanvasId.take(&canvasElement);

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:173
> +	   m_frontendDispatcher->canvasRemoved(identifier);

I think that it might be possible for this to run even after `disable()` has
been called.  I think that either this should be guarded by a `m_enabled` check
or `m_timer.stop()` should be called in `disable()`.


More information about the webkit-reviews mailing list