Bug 174482

Summary: Web Inspector: Record actions performed on CanvasRenderingContext2D
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, commit-queue, esprehn+autocc, gyuyoung.kim, inspector-bugzilla-changes, joepeck, kondapallykalyan, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 174278, 174481    
Bug Blocks: 173807, 174484    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews113 for mac-elcapitan
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Patch
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews113 for mac-elcapitan
none
Patch
none
Patch
none
Patch none

Description Devin Rousso 2017-07-13 18:33:30 PDT
Use the protocol/instrumentation logic created in <https://webkit.org/b/174481> Web Inspector: create protocol functions for recording Canvas contexts
Comment 1 Devin Rousso 2017-07-26 14:58:02 PDT
Created attachment 316487 [details]
Patch
Comment 2 Joseph Pecoraro 2017-07-26 16:04:12 PDT
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.
Comment 3 Devin Rousso 2017-07-26 16:28:17 PDT
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.
Comment 4 Joseph Pecoraro 2017-07-26 16:44:09 PDT
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\//, "");
    }
Comment 5 Devin Rousso 2017-07-26 21:03:49 PDT
Created attachment 316515 [details]
Patch
Comment 6 Devin Rousso 2017-07-27 10:20:22 PDT
Created attachment 316555 [details]
Patch
Comment 7 Devin Rousso 2017-07-27 17:36:40 PDT
Created attachment 316585 [details]
Patch
Comment 8 Build Bot 2017-07-27 18:28:07 PDT
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
Comment 9 Build Bot 2017-07-27 18:28:09 PDT
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
Comment 10 Build Bot 2017-07-27 18:49:22 PDT
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
Comment 11 Build Bot 2017-07-27 18:49:23 PDT
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
Comment 12 Build Bot 2017-07-27 18:50:23 PDT
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
Comment 13 Build Bot 2017-07-27 18:50:25 PDT
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
Comment 14 Joseph Pecoraro 2017-07-27 19:41:16 PDT
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.
Comment 15 Devin Rousso 2017-07-27 21:00:10 PDT
Created attachment 316610 [details]
Patch
Comment 16 Devin Rousso 2017-07-27 21:01:08 PDT
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.
Comment 17 Build Bot 2017-07-27 22:07:48 PDT
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
Comment 18 Build Bot 2017-07-27 22:07:50 PDT
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
Comment 19 Build Bot 2017-07-27 22:12:00 PDT
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
Comment 20 Build Bot 2017-07-27 22:12:01 PDT
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
Comment 21 Build Bot 2017-07-27 22:27:46 PDT
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
Comment 22 Build Bot 2017-07-27 22:27:47 PDT
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
Comment 23 Devin Rousso 2017-07-27 22:35:44 PDT
Created attachment 316616 [details]
Patch
Comment 24 Build Bot 2017-07-27 22:40:21 PDT
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
Comment 25 Devin Rousso 2017-07-27 22:42:37 PDT
Created attachment 316619 [details]
Patch
Comment 26 Devin Rousso 2017-07-27 23:34:32 PDT
Created attachment 316623 [details]
Patch
Comment 27 Joseph Pecoraro 2017-07-28 11:11:41 PDT
Comment on attachment 316623 [details]
Patch

r=me
Comment 28 WebKit Commit Bot 2017-07-28 11:22:21 PDT
Comment on attachment 316623 [details]
Patch

Clearing flags on attachment: 316623

Committed r220008: <http://trac.webkit.org/changeset/220008>
Comment 29 WebKit Commit Bot 2017-07-28 11:22:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Radar WebKit Bug Importer 2017-07-28 12:39:38 PDT
<rdar://problem/33596140>