Bug 173807 – Web Inspector: [META] Allow canvas actions to be recorded/replayed

• Status: ASSIGNED
• Component: Web Inspector

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 313783 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 313853 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 313891 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 313989 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 313990 [details]
[Video] After Patch is applied

[Joseph Pecoraro]

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.

[Devin Rousso]

Created attachment 314117 [details]
[Patch] WIP

[Radar WebKit Bug Importer]

<rdar://problem/33122149>

[Devin Rousso]

Created attachment 314854 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 314962 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 315200 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 315897 [details]
[Patch] WIP

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Devin Rousso]

Created attachment 315914 [details]
[Patch] WIP

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Devin Rousso]

Created attachment 316135 [details]
[Patch] WIP

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Sam Weinig]

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?

[Devin Rousso]

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

[Devin Rousso]

Created attachment 316178 [details]
[Patch] WIP

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Sam Weinig]

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

[Devin Rousso]

Created attachment 316242 [details]
[Patch] Attempt Modifying IDL

[Build Bot]

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.

[Devin Rousso]

Created attachment 316254 [details]
[Patch] Attempt Modifying IDL

[Devin Rousso]

Created attachment 316267 [details]
[Patch] Attempt Modifying IDL

[Devin Rousso]

Created attachment 316269 [details]
[Patch] Attempt Modifying IDL

[Devin Rousso]

Created attachment 316270 [details]
[Patch] Attempt Modifying IDL

[Blaze Burg]

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.

[Blaze Burg]

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.

[Blaze Burg]

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

[Devin Rousso]

Created attachment 316315 [details]
[Patch] Modifying IDL

[Joseph Pecoraro]

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.

[Blaze Burg]

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.