Bug 119776 – Don't use ScriptProfiler to find canvases for instrumentation

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Dean Jackson]

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.

[Radar WebKit Bug Importer]

<rdar://problem/14730401>

[Dean Jackson]

Created attachment 208690 [details]
Patch

[Joseph Pecoraro]

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]

Committed r154033: <http://trac.webkit.org/changeset/154033>