Bug 119776 - Don't use ScriptProfiler to find canvases for instrumentation
Summary: Don't use ScriptProfiler to find canvases for instrumentation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-08-13 16:54 PDT by Dean Jackson
Modified: 2013-08-13 18:03 PDT (History)
6 users (show)

See Also:


Attachments
Patch (6.01 KB, patch)
2013-08-13 17:03 PDT, Dean Jackson
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 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.
Comment 1 Radar WebKit Bug Importer 2013-08-13 16:54:20 PDT
<rdar://problem/14730401>
Comment 2 Dean Jackson 2013-08-13 17:03:19 PDT
Created attachment 208690 [details]
Patch
Comment 3 Joseph Pecoraro 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.
Comment 4 Dean Jackson 2013-08-13 18:03:03 PDT
Committed r154033: <http://trac.webkit.org/changeset/154033>