RESOLVED FIXED 119776
Don't use ScriptProfiler to find canvases for instrumentation
https://bugs.webkit.org/show_bug.cgi?id=119776
Summary Don't use ScriptProfiler to find canvases for instrumentation
Dean Jackson
Reported 2013-08-13 16:54:00 PDT
InspectorCanvasAgent::findFramesWithUninstrumentedCanvases uses a ScriptProfiler to walk the tree looking for canvas elements. This is currently not implemented in JSC, but we can do this directly with DOM methods. We're only looking for Canvas elements that have a context, so there isn't a need for this abstract walking object.
Attachments
Patch (6.01 KB, patch)
2013-08-13 17:03 PDT, Dean Jackson
joepeck: review+
Radar WebKit Bug Importer
Comment 1 2013-08-13 16:54:20 PDT
Dean Jackson
Comment 2 2013-08-13 17:03:19 PDT
Joseph Pecoraro
Comment 3 2013-08-13 17:26:00 PDT
Comment on attachment 208690 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208690&action=review r=me > Source/WebCore/ChangeLog:10 > + InspectorCanvasAgent::findFramesWithUninstrumentedCanvases uses a ScriptProfiler to walk the tree > + looking for canvas elements. This is currently not implemented in JSC, but we can do this directly > + with DOM methods. We're only looking for Canvas elements that have a context, so there isn't a need > + for this abstract walking object. So we discussed this on IRC and this doesn't take into account -webkit-canvas(…) CSS Canvases or even Shadow DOM Canvases if they existed. I don't know if the original code handled those, but it is something that we should eventually handle. I think you could add a FIXME in source code. > Source/WebCore/html/HTMLCanvasElement.h:194 > +inline bool isHTMLCanvasElement(Node* node) > +{ > + return node->hasTagName(HTMLNames::canvasTag); > +} This one shouldn't be needed, the const one should work just fine with non-const Node*'s passed in. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:270 > + if (!frame->document() || frame->page() != m_pageAgent->page()) I suspect the `page != pageAgent->page` check at the end of this can be removed now that we are walking this explicit page's frame tree. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:278 > + if (canvas->renderingContext()) > + m_framesWithUninstrumentedCanvases.set(frame, true); You can `break;` inside this if statement to break out of this inner for loop.
Dean Jackson
Comment 4 2013-08-13 18:03:03 PDT
Note You need to log in before you can comment on or make changes to this bug.