Bug 174482

Created attachment 316487 [details] Patch

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 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 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\//, ""); }

Created attachment 316515 [details] Patch

Created attachment 316555 [details] Patch

Created attachment 316585 [details] Patch

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

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 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

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 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

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 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.

Created attachment 316610 [details] Patch

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 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

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 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

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 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

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

Created attachment 316616 [details] Patch

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

Created attachment 316619 [details] Patch

Created attachment 316623 [details] Patch

Comment on attachment 316623 [details] Patch r=me

Comment on attachment 316623 [details] Patch Clearing flags on attachment: 316623 Committed r220008: <http://trac.webkit.org/changeset/220008>

All reviewed patches have been landed. Closing bug.

<rdar://problem/33596140>