Since <https://webkit.org/b/138941> we are able to show all active contexts on the page. We should also be able to record all actions made to that canvas in the next active frame.
Created attachment 313783 [details] [Patch] WIP
Created attachment 313853 [details] [Patch] WIP
Created attachment 313891 [details] [Patch] WIP
Created attachment 313989 [details] [Patch] WIP
Created attachment 313990 [details] [Video] After Patch is applied
Comment on attachment 313989 [details] [Patch] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=313989&action=review Just skimmed the backend. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1058 > +static inline String stringForWindingRule(CanvasRenderingContext2D::WindingRule windingRule) The `inline` here should not be needed. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1068 > + return ""; Could return just `String()` which is a tiny bit cheaper than `""`. In any case, this shouldn't be reached ;). > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1532 > + [&] (RefPtr<HTMLImageElement>& imageElement) { > + source = imageElement->currentSrc().string(); > + }, > + [&] (RefPtr<HTMLVideoElement>& videoElement) { > + // get dataURL of current frame > + source = videoElement->currentSrc().string(); > + }, We probably can't depend on src alone. An image might be a `app://my-image` only accessible by the inspected page or an animated image at a particular frame. A video might be at a particular time / frame in the video. It would be best to get a dataURL of what is showing. > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:218 > + static inline String stringForImageSmoothingQuality(ImageSmoothingQuality quality) No need for `inline` again. But this probably doesn't need to be inline or in the header. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:323 > + bool isRadial = canvasGradient.gradient().isRadial(); Every use of the gradient is `canvasGradient.gradient()`. Might read better to pull that out into a local. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:325 > + object->setString("type", isRadial ? "radial-gradient" : "linear-gradient"); ASCIILiteral > Source/WebCore/inspector/InspectorCanvasAgent.cpp:393 > + RefPtr<InspectorObject> attributes = InspectorObject::create(); > + RefPtr<InspectorObject> functions = InspectorObject::create(); I think a bunch of these which don't expect to move beyond this function can be Ref<> instead of RefPtr<>. I'd expect the usage to be the same but its just clearer that this won't be passed around. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:480 > + [&] (unsigned value) { parametersData->addItem(static_cast<int>(value)); }, Should we assert that the `unsigned` value is not > INT_MAX? > Source/WebCore/page/PageConsoleClient.cpp:217 > +void PageConsoleClient::record(JSC::ExecState*, Ref<ScriptArguments>&& arguments) `console.record` should be a separate bug + patch.
Created attachment 314117 [details] [Patch] WIP
<rdar://problem/33122149>
Created attachment 314854 [details] [Patch] WIP
Created attachment 314962 [details] [Patch] WIP
Created attachment 315200 [details] [Patch] WIP
Created attachment 315897 [details] [Patch] WIP
Comment on attachment 315897 [details] [Patch] WIP Attachment 315897 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4146242 New failing tests: inspector/canvas/recording-2d.html fast/canvas/2d.setTransform.matrix.html
Created attachment 315903 [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 315897 [details] [Patch] WIP Attachment 315897 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4146245 New failing tests: inspector/canvas/recording-2d.html fast/canvas/2d.setTransform.matrix.html
Created attachment 315904 [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 315897 [details] [Patch] WIP Attachment 315897 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4146307 New failing tests: fast/canvas/2d.setTransform.matrix.html
Created attachment 315907 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 315897 [details] [Patch] WIP Attachment 315897 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4146282 New failing tests: inspector/canvas/recording-2d.html fast/canvas/2d.setTransform.matrix.html
Created attachment 315908 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 315914 [details] [Patch] WIP
Attachment 315914 [details] did not pass style-queue: ERROR: Source/WebCore/inspector/InspectorCanvasAgent.cpp:588: Omit int when using unsigned [runtime/unsigned] [1] ERROR: Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:4915: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WebCore/inspector/InspectorCanvasActionTypes.h:55: Omit int when using unsigned [runtime/unsigned] [1] Total errors found: 3 in 70 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 315914 [details] [Patch] WIP Attachment 315914 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4147462 New failing tests: imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html
Created attachment 315915 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Created attachment 316135 [details] [Patch] WIP
Attachment 316135 [details] did not pass style-queue: ERROR: Source/WebCore/inspector/InspectorCanvas.cpp:43: "ImageData.h" already included at Source/WebCore/inspector/InspectorCanvas.cpp:42 [build/include] [4] Total errors found: 1 in 77 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 316135 [details] [Patch] WIP Attachment 316135 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4164287 New failing tests: inspector/canvas/recording-2d.html
Created attachment 316148 [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 316135 [details] [Patch] WIP Attachment 316135 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4164347 New failing tests: inspector/canvas/recording-2d.html
Created attachment 316150 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 316135 [details] [Patch] WIP Attachment 316135 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4164331 New failing tests: inspector/canvas/recording-2d.html
Created attachment 316152 [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
Perhaps this has already been discussed, but it seems unfortunate for us to have graphics recording at two layers. At the CanvasRenderingContext2D layer, this new work, and at the GraphicsContext layer, DisplayList::Recorder. Can they be consolidated in anyway? This also adds a ton of complexity to the canvas code. If the goal is to trace the calls to the canvas operations, perhaps this could be generated from the IDL rather than added manually?
(In reply to Sam Weinig from comment #33) > Perhaps this has already been discussed, but it seems unfortunate for us to > have graphics recording at two layers. At the CanvasRenderingContext2D > layer, this new work, and at the GraphicsContext layer, > DisplayList::Recorder. Can they be consolidated in anyway? I am unfamiliar with DisplayList::Recorder, but I think that it's too low level for our needs. The idea for CanvasRenderingContext2D is that we want to be able to be able to replicate every call made to the context exactly as it was called in JavaScript, meaning that we can then replay it in a regular webpage. > This also adds a ton of complexity to the canvas code. If the goal is to > trace the calls to the canvas operations, perhaps this could be generated > from the IDL rather than added manually? That's a great idea, but it would probably be better suited as a followup. I am more concerned with getting the initial feature working and available for people to use, but I do think that would make things a lot easier.
Created attachment 316178 [details] [Patch] WIP
Comment on attachment 316178 [details] [Patch] WIP Attachment 316178 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4166458 Number of test failures exceeded the failure limit.
Created attachment 316184 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 316178 [details] [Patch] WIP Attachment 316178 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4166462 Number of test failures exceeded the failure limit.
Created attachment 316189 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 316178 [details] [Patch] WIP Attachment 316178 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4166476 Number of test failures exceeded the failure limit.
Created attachment 316191 [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 316178 [details] [Patch] WIP Attachment 316178 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4166603 New failing tests: imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html
Created attachment 316192 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
(In reply to Devin Rousso from comment #34) > (In reply to Sam Weinig from comment #33) > > Perhaps this has already been discussed, but it seems unfortunate for us to > > have graphics recording at two layers. At the CanvasRenderingContext2D > > layer, this new work, and at the GraphicsContext layer, > > DisplayList::Recorder. Can they be consolidated in anyway? > I am unfamiliar with DisplayList::Recorder, but I think that it's too low > level for our needs. The idea for CanvasRenderingContext2D is that we want > to be able to be able to replicate every call made to the context exactly as > it was called in JavaScript, meaning that we can then replay it in a regular > webpage. I think having a broader discussion before doing something so close to what already exists still makes sense. > > This also adds a ton of complexity to the canvas code. If the goal is to > > trace the calls to the canvas operations, perhaps this could be generated > > from the IDL rather than added manually? > That's a great idea, but it would probably be better suited as a followup. > I am more concerned with getting the initial feature working and available > for people to use, but I do think that would make things a lot easier. I don't think that it is the kind of thing that makes sense as a follow up. This feature is quite a bit of new manually written code that places a burden on any additional feature work on canvas. Before add the feature, we should be sure the design scales well, and I think that probably means generating it.
Created attachment 316242 [details] [Patch] Attempt Modifying IDL
Attachment 316242 [details] did not pass style-queue: ERROR: Source/WebCore/inspector/InspectorInstrumentation.h:34: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 81 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 316254 [details] [Patch] Attempt Modifying IDL
Created attachment 316267 [details] [Patch] Attempt Modifying IDL
Created attachment 316269 [details] [Patch] Attempt Modifying IDL
Created attachment 316270 [details] [Patch] Attempt Modifying IDL
Comment on attachment 316270 [details] [Patch] Attempt Modifying IDL View in context: https://bugs.webkit.org/attachment.cgi?id=316270&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:40 > + InspectorRecording=recordCanvasAction, Maybe we could call this "CallTracingCallback" or something like that, and pass in a generic static method that bounces to InspectorInstrumentation. We'd probably want different methods for different types of recordings.
Comment on attachment 316270 [details] [Patch] Attempt Modifying IDL View in context: https://bugs.webkit.org/attachment.cgi?id=316270&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4679 > + push(@$outputArray, " if (UNLIKELY(impl.inspectorRecordingActive()))\n"); Nit: doesn't need to be named for Inspector. Just callTracingActive() would be cool, IMO. I want to do this for various DOM APIs eventually. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4889 > + AddToImplIncludes("InspectorInstrumentation.h"); I'd like to avoid pulling in this header, as any changes to instrumentation header would then cause rebuilding of dependents. This might be not so bad in theory, but in practice it's hard to change/rebuild just one IDL file due to build system bugs. If we are serious about making this generic (Sam, WDYT?), you can call some generic method on a singleton like CallTracer::traceFunctionCall(traceGroup, impl, $name, value). traceGroup could be what is passed as argument to the IDL extended attribute. Then you can add InspectorInstrumentation hooks inside of that. This also avoids pulling in InspectorInstrumentation to all generated files. > Source/WebCore/bindings/scripts/IDLAttributes.json:215 > + }, You'll need to add test cases for the code generator changes so that I can review what the generated code looks like when the attribute is set. You'll need a test case for each of the places the attribute is allowed in IDL: interface, attribute, and operation.
(In reply to Sam Weinig from comment #44) > (In reply to Devin Rousso from comment #34) > > (In reply to Sam Weinig from comment #33) > > > Perhaps this has already been discussed, but it seems unfortunate for us to > > > have graphics recording at two layers. At the CanvasRenderingContext2D > > > layer, this new work, and at the GraphicsContext layer, > > > DisplayList::Recorder. Can they be consolidated in anyway? > > I am unfamiliar with DisplayList::Recorder, but I think that it's too low > > level for our needs. The idea for CanvasRenderingContext2D is that we want > > to be able to be able to replicate every call made to the context exactly as > > it was called in JavaScript, meaning that we can then replay it in a regular > > webpage. > > I think having a broader discussion before doing something so close to what > already exists still makes sense. There's no way to go from a DisplayList ItemType back to the original canvas2d API calls, short of saving everything about the JS API call (which is what the current patch does!). Additionally, we want to preserve calls that are invalid / don't do anything; a display list may (if it's smart) prune such things. I don't understand how a dev-facing feature could be based on this approach. > > > This also adds a ton of complexity to the canvas code. If the goal is to > > > trace the calls to the canvas operations, perhaps this could be generated > > > from the IDL rather than added manually? > > That's a great idea, but it would probably be better suited as a followup. > > I am more concerned with getting the initial feature working and available > > for people to use, but I do think that would make things a lot easier. > > I don't think that it is the kind of thing that makes sense as a follow up. > This feature is quite a bit of new manually written code that places a > burden on any additional feature work on canvas. Before add the feature, we > should be sure the design scales well, and I think that probably means > generating it. I tend to agree with Sam, though the bindings generator is no easy thing to hack on. If we make this generic enough, then inspector-specific changes are limited to adding the extended attribute to more things, and adding instrumentation hooks in the generic hook thing rather than in generated code directly.
Created attachment 316315 [details] [Patch] Modifying IDL
Comment on attachment 316315 [details] [Patch] Modifying IDL View in context: https://bugs.webkit.org/attachment.cgi?id=316315&action=review > Source/WebCore/bindings/scripts/test/TestCallTracer.idl:35 > +[ > + CallTracingCallback=testCallTracerInterface, > +] interface TestCallTracer { > + [CallTracingCallback=testCallTracerAttribute] attribute boolean attributeTestCallTracer; > + > + [CallTracingCallback=testCallTracerOperation] void operationTestCallTracerNoArguments(); > + [CallTracingCallback=testCallTracerOperation] void operationTestCallTracerWithArguments(Node nodeArg); > + [CallTracingCallback=testCallTracerOperation] void operationTestCallTracerWithNullableArgument(Node? nodeNullableArg); > + [CallTracingCallback=testCallTracerOperation] void operationTestCallTracerWithVariadicArgument(Node... nodeVariadicArg); > +}; The IDLAttributes.json describes CallTrackingCallback as being supported on [interface, attribute, operation]. This doesn't show that putting it on the interface does it by default for ALL attributes/operations. So it works in all cases: > Interface attribute handles all getters/setters/operations: > > [ > CallTracingCallback=testCallTracerInterface, > ] interface TestCallTracer { > attribute boolean attr; > void test(Node node); > }; > > Per-attribute / per-operation > > interface TestCallTracer { > attribute boolean unaffected; > void unaffectedTest(Node node); > > [CallTracingCallback=testCallTracerAttribute] attribute boolean attr; > [CallTracingCallback=testCallTracerOperation] void test(Node node); > }; > > Mixed: > > [ > CallTracingCallback=testCallTracerInterface, > ] interface TestCallTracer { > attribute boolean defaultAttr; > [CallTracingCallback=testCallTracerAttribute] attribute boolean specificAttr; > > void defaultTest(Node node); > [CallTracingCallback=testCallTracerOperation] void specificTest(Node node); > }; I think you can probably just turn this into a Mixed test and include some attributes / operations that will get the Interface's call tracer callback.
Obsoleting patches, this work landed as part of https://bugs.webkit.org/show_bug.cgi?id=174482. This is just a tracking bug at this point.