[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