Bug 174482 – Web Inspector: Record actions performed on CanvasRenderingContext2D

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Use the protocol/instrumentation logic created in <https://webkit.org/b/174481> Web Inspector: create protocol functions for recording Canvas contexts

[Devin Rousso]

Created attachment 316487 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 316487 [details]
Patch

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

r- because compiles failed.

> LayoutTests/inspector/canvas/recording-2d-expected.txt:1617
> +    "data:,",
> +    "testB",

Is the empty data: value expected for the createPattern from a video? And probably everything else trying tog et image data from a video.

Can you add a test that actually gets image data from a video? It doesn't need to be part of this large thing, it can be a very targeted test given its a specific scenario.

> LayoutTests/inspector/canvas/recording-2d.html:45
> +function e(f){
> +    try { f(); } catch(e) { }
> +}

This name is rather cryptic. `ignoreException` would be better, but I think you wanted it short to avoid clutter down below?

> LayoutTests/inspector/canvas/recording-2d.html:370
> +        () => {

I think there should be a test for a function call with NaN to see what that produces:

    ctx.setLineWidth(NaN)

Since NaN is not JSON stringifyable. It seems right now we produce `null` but we should have a test for its behavior which we can see changes when we fix that issue (if we decide to address this edge case).

> LayoutTests/inspector/canvas/recording-2d.html:386
> +    function requestRecording(parameters, resolve, reject) {

Instead of an array of parameters you could do:

    {singleFrame, memoryLimit}

Having a value of `undefined` should be the same as the default.

> LayoutTests/inspector/canvas/recording-2d.html:404
> +                // The bots will fail since the path locally is most likely different than the bots.

What does this mean?

> LayoutTests/inspector/canvas/recording-2d.html:411
> +        CanvasAgent.requestRecording(canvases[0].identifier, ...parameters, (error) => {

You'd then just pass the params here and it would be clear:

    CanvasAgent.requestRecording(canvases[0].identifier, singleFrame, memoryLimit, (error) => { ... });

> LayoutTests/inspector/canvas/recording-2d.html:420
> +            InspectorTest.evaluateInPage(`performActions()`, (error) => {
> +                if (error)
> +                    reject(error);
> +            });

Lets just do: InspectorTest.evaluateInPage(`performActions()`); We don't expect these to error for simple cases like this, and if they did the test page would still report it (and I think try to stop).

> LayoutTests/inspector/canvas/recording-2d.html:466
> +    suite.addTestCase({
> +        name: "Canvas.requestRecording.InvalidCanvasId",

These tests can go into a more basic test:

    + LayoutTests/inspector/canvas/requestRecording.html
    + LayoutTests/inspector/canvas/cancelRecording.html
    * LayoutTests/inspector/canvas/recording-2d.html

Since these are not specific to 2d at all. Its also weird to bury them in here.

[Devin Rousso]

Comment on attachment 316487 [details]
Patch

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

Not really sure why it isn't building.  It works fine on my computer :(

>> LayoutTests/inspector/canvas/recording-2d-expected.txt:1617
>> +    "testB",
> 
> Is the empty data: value expected for the createPattern from a video? And probably everything else trying tog et image data from a video.
> 
> Can you add a test that actually gets image data from a video? It doesn't need to be part of this large thing, it can be a very targeted test given its a specific scenario.

Yes the empty "data:," is from the <video>.  My issue with using an actual video is that I'd need something that has the same content in every frame in order to not introduce possible flakiness.  Also, QuickTime doesn't let me make a super small video recording (I think the minimum is 20x20), so the base64 would be much larger and annoying.  Are there sample video resources anywhere that I could use?

>> LayoutTests/inspector/canvas/recording-2d.html:45
>> +}
> 
> This name is rather cryptic. `ignoreException` would be better, but I think you wanted it short to avoid clutter down below?

Yes I was trying to avoid clutter.  I can change it to `ignoreException`, but I figured that since the exception isn't really the important part of the test (we don't care about the effect of the action, just that it was recorded correctly) it was fine being short.

>> LayoutTests/inspector/canvas/recording-2d.html:386
>> +    function requestRecording(parameters, resolve, reject) {
> 
> Instead of an array of parameters you could do:
> 
>     {singleFrame, memoryLimit}
> 
> Having a value of `undefined` should be the same as the default.

I was not aware of that.  I will change it.

>> LayoutTests/inspector/canvas/recording-2d.html:404
>> +                // The bots will fail since the path locally is most likely different than the bots.
> 
> What does this mean?

When I run the test locally I get "/Volumes/DATA/OpenSource/LayoutTests/inspector/canvas/recording-2d.html".  The bot's don't follow the "/Volumes/Data/OpenSource/" part of the path, so they'd FAIL since the strings don't match.

[Joseph Pecoraro]

Comment on attachment 316487 [details]
Patch

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

>>> LayoutTests/inspector/canvas/recording-2d.html:404
>>> +                // The bots will fail since the path locally is most likely different than the bots.
>> 
>> What does this mean?
> 
> When I run the test locally I get "/Volumes/DATA/OpenSource/LayoutTests/inspector/canvas/recording-2d.html".  The bot's don't follow the "/Volumes/Data/OpenSource/" part of the path, so they'd FAIL since the strings don't match.

Ahh so this doesn't have anything to do with bots. This will be different on every person's machines. I don't think the comment is very useful. You could name a function `sanitizePaths` and it would be clearer. Other tests have something like:

    function sanitizeURL(url) {
        return url.replace(/^.*?LayoutTests\//, "");
    }

[Devin Rousso]

Created attachment 316515 [details]
Patch

[Devin Rousso]

Created attachment 316555 [details]
Patch

[Devin Rousso]

Created attachment 316585 [details]
Patch

[Build Bot]

Comment on attachment 316585 [details]
Patch

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

New failing tests:
inspector/canvas/recording-2d.html

[Build Bot]

Created attachment 316591 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316585 [details]
Patch

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

New failing tests:
inspector/canvas/recording-2d.html

[Build Bot]

Created attachment 316595 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316585 [details]
Patch

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

New failing tests:
inspector/canvas/recording-2d.html

[Build Bot]

Created attachment 316596 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Comment on attachment 316585 [details]
Patch

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

> Source/WebCore/bindings/js/CallTracerTypes.h:67
> +> RecordCanvasActionVariant;

You should mention this file in the ChangeLog and explain why these changes were necessary.

It seems older mac SDKs were unable to build successfully if there were Variants with Variants, so you worked around this by unrolling them and extracting the values in generated code.

[Devin Rousso]

Created attachment 316610 [details]
Patch

[Devin Rousso]

Comment on attachment 316585 [details]
Patch

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

>> Source/WebCore/bindings/js/CallTracerTypes.h:67
>> +> RecordCanvasActionVariant;
> 
> You should mention this file in the ChangeLog and explain why these changes were necessary.
> 
> It seems older mac SDKs were unable to build successfully if there were Variants with Variants, so you worked around this by unrolling them and extracting the values in generated code.

I uploaded the wrong version of the last patch.  Apologies for the confusion.  The new patch should have a comment to this effect.

[Build Bot]

Comment on attachment 316610 [details]
Patch

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

New failing tests:
inspector/canvas/recording-2d.html

[Build Bot]

Created attachment 316613 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316610 [details]
Patch

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

New failing tests:
inspector/canvas/recording-2d.html

[Build Bot]

Created attachment 316614 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316610 [details]
Patch

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

New failing tests:
inspector/canvas/recording-2d.html

[Build Bot]

Created attachment 316615 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Devin Rousso]

Created attachment 316616 [details]
Patch

[Build Bot]

Comment on attachment 316616 [details]
Patch

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

New failing tests:
(JS) JSTestCallTracer.cpp

[Devin Rousso]

Created attachment 316619 [details]
Patch

[Devin Rousso]

Created attachment 316623 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 316623 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 316623 [details]
Patch

Clearing flags on attachment: 316623

Committed r220008: <http://trac.webkit.org/changeset/220008>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33596140>