Bug 105721 – Web Inspector: [Canvas] report if there is any uninstrumented canvas on a page

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Andrey Adaikin]

Adding a method to the protocol to report if there is any uninstrumented canvas on a page.

Patch to follow.

[Andrey Adaikin]

Created attachment 180672 [details]
Patch

[Andrey Adaikin]

Created attachment 180705 [details]
Added a test

[Build Bot]

Comment on attachment 180705 [details]
Added a test

Attachment 180705 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15531003

New failing tests:
inspector/profiler/canvas2d/canvas-has-uninstrumented-canvases.html

[Build Bot]

Comment on attachment 180705 [details]
Added a test

Attachment 180705 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15503697

New failing tests:
inspector/profiler/canvas2d/canvas-has-uninstrumented-canvases.html

[Yury Semikhatsky]

Comment on attachment 180705 [details]
Added a test

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

> Source/WebCore/inspector/Inspector.json:3194
>                  "description": "Unique object identifier."

The description should be updated for this type.

> Source/WebCore/inspector/Inspector.json:3243
> +                "name": "hasUninstrumentedCanvases",

Add a description?

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:96
> +    ensureHasUninstrumentedCanvasesCalculated();

Why do you need this call in enable method? Can it be done from hasUninstrumentedCanvases request?

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:221
> +void InspectorCanvasAgent::ensureHasUninstrumentedCanvasesCalculated()

ensureHasUninstrumentedCanvasesCalculated -> ensureHasUninstrumentedCanvasesInitialized

> Source/WebCore/inspector/InspectorCanvasAgent.h:100
> +    typedef HashMap<Frame*, bool> FrameToHasUninstrumentedCanvasesResult;

Why do you need to store this map? I think running through all wrappers would be fast enough to do on each hasUninstrumentedCanvases, wdyt?

[Andrey Adaikin]

Comment on attachment 180705 [details]
Added a test

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

>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:96
>> +    ensureHasUninstrumentedCanvasesCalculated();
> 
> Why do you need this call in enable method? Can it be done from hasUninstrumentedCanvases request?

The initialization has to be done before we start wrapping canvas contexts by the means of this Agent. Thus, at the initialization time, if there is any canvas element, for which the getContext() method has been already called, we say we have a uninstrumented canvas (because it was not wrapped by this Agent). By the time the hasUninstrumentedCanvases is called, we may well have a canvas context wrapped already.

>> Source/WebCore/inspector/InspectorCanvasAgent.h:100
>> +    typedef HashMap<Frame*, bool> FrameToHasUninstrumentedCanvasesResult;
> 
> Why do you need to store this map? I think running through all wrappers would be fast enough to do on each hasUninstrumentedCanvases, wdyt?

We need to save early results (i.e. before we start wrapping canvas contexts by this Agent). The map from a Frame to bool will be needed when we support iframes (exec context ID).

[Yury Semikhatsky]

Comment on attachment 180705 [details]
Added a test

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

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:118
> +    ensureHasUninstrumentedCanvasesCalculated();

You don't need this call as you check if the agent is enabled and enable method finds all uninstrumented canvases.

> Source/WebCore/inspector/InspectorCanvasAgent.h:101
> +    FrameToHasUninstrumentedCanvasesResult m_hasUninstrumentedCanvasesResults;

This can be a set FramesWithUninstrumentedCanvases.

[Andrey Adaikin]

Comment on attachment 180705 [details]
Added a test

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

>> Source/WebCore/inspector/Inspector.json:3194
>>                  "description": "Unique object identifier."
> 
> The description should be updated for this type.

Done.

>> Source/WebCore/inspector/Inspector.json:3243
>> +                "name": "hasUninstrumentedCanvases",
> 
> Add a description?

Done.

>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:118
>> +    ensureHasUninstrumentedCanvasesCalculated();
> 
> You don't need this call as you check if the agent is enabled and enable method finds all uninstrumented canvases.

Done.

>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:221
>> +void InspectorCanvasAgent::ensureHasUninstrumentedCanvasesCalculated()
> 
> ensureHasUninstrumentedCanvasesCalculated -> ensureHasUninstrumentedCanvasesInitialized

renamed to findFramesWithUninstrumentedCanvases

>> Source/WebCore/inspector/InspectorCanvasAgent.h:101
>> +    FrameToHasUninstrumentedCanvasesResult m_hasUninstrumentedCanvasesResults;
> 
> This can be a set FramesWithUninstrumentedCanvases.

Done.

[Andrey Adaikin]

Created attachment 180781 [details]
Patch to land

[Andrey Adaikin]

Committed r138497: <http://trac.webkit.org/changeset/138497>

[Csaba Osztrogonác]

(In reply to comment #10)
> Committed r138497: <http://trac.webkit.org/changeset/138497>

The new test fails on Qt: http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r138497%20%2846350%29/inspector/profiler/canvas2d/canvas-has-uninstrumented-canvases-diff.txt

 ... maybe on other platforms too, but I won't check any other platform 
in my xmas holiday. Could you check and fix it, please?

[Andrey Adaikin]

(In reply to comment #11)
> (In reply to comment #10)
> > Committed r138497: <http://trac.webkit.org/changeset/138497>
> 
> The new test fails on Qt: http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r138497%20%2846350%29/inspector/profiler/canvas2d/canvas-has-uninstrumented-canvases-diff.txt
> 
>  ... maybe on other platforms too, but I won't check any other platform 
> in my xmas holiday. Could you check and fix it, please?

Committed r138498: <http://trac.webkit.org/changeset/138498>

Sorry, forgot to skip the test on QT.