Web Inspector: provide method for recording CanvasRenderingContext2D from JavaScript

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

This would follow a similar process to console.time()/console.timeEnd().

    console.record(...);
    ...
    console.recordEnd(...);

Currently, it would

[Devin Rousso]

(In reply to Devin Rousso from comment #0)
> Currently, it would

Oops. :P

[Devin Rousso]

Created attachment 317219 [details]
Patch

[BJ Burg]

Comment on attachment 317219 [details]
Patch

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

Overall the patch is good with a few issues. And it looks like you pulled in some wrong commits into the diff, please fix that.

> Source/WebInspectorUI/ChangeLog:14
> +        Open a Recording tab if no CanvasContentView triggered a recording, as means that the

What will happen if a page samples every 10'th frame? The UI will be unusable. I think if a recording is already being viewed, then the new ones should show up in the navigation sidebar list of recordings (that we don't have yet). Let's remember this when working on that UI.

> Source/WebCore/inspector/InspectorInstrumentation.h:1142
> +inline void InspectorInstrumentation::didRequestRecordingCanvas(HTMLCanvasElement& canvasElement)

I don't like this. Let's follow the pattern used for startProfiling / stopProfiling:

InspectorInstrumentation::{start,stop}CanvasRecording
 -> calls canvasAgent->{start,stop}FromConsole

This is much clearer when reading the canvas agent code because we know exactly which entry point is being used by console API without digging up 5 other files.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:126
> +        let defaultPrevented = this.dispatchEventToListeners(WI.CanvasManager.Event.RecordingFinished, {canvas, recording});

I don't understand why this change is needed.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:131
> +            WI.showRepresentedObject(recording, null, {suppressSelection: true});

Likewise. If this is showing a recording after console.recordEnd, it would be much easier to grok if the logic was inside InspectorCanvasAgent::stopRecordingFromConsole.

> Tools/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=174718

Oops.

> LayoutTests/ChangeLog:3
> +        Web Inspector: add stack trace information for each RecordingAction

Oops.

[Radar WebKit Bug Importer]

<rdar://problem/34040740>

[Maciej Stachowiak]

(In reply to Devin Rousso from comment #0)
> This would follow a similar process to console.time()/console.timeEnd().
> 
>     console.record(...);
>     ...
>     console.recordEnd(...);
> 
> Currently, it would

We probably shouldn't use such generic names for this feature. There's other things you could imagine wanting to "record" and console is sort of shared namespace with other browsers that is partly standardized. So if we add this, it should probably mention "canvas" in the names somehow.

[Devin Rousso]

Created attachment 325277 [details]
Patch

[Build Bot]

Comment on attachment 325277 [details]
Patch

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

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

[Build Bot]

Created attachment 325279 [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 325277 [details]
Patch

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

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

[Build Bot]

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

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

[Build Bot]

Comment on attachment 325277 [details]
Patch

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

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

[Build Bot]

Created attachment 325281 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

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

[Build Bot]

Comment on attachment 325277 [details]
Patch

Attachment 325277 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5028158

New failing tests:
js/console.html

[Build Bot]

Created attachment 325282 [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.6

[Devin Rousso]

Created attachment 325484 [details]
Patch

Attempt for bots.

[Build Bot]

Comment on attachment 325484 [details]
Patch

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

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

[Build Bot]

Created attachment 325499 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

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

[Devin Rousso]

Created attachment 325500 [details]
Patch

Gotta love spelling errors ᕕ( ᐛ )ᕗ

[Devin Rousso]

Created attachment 325613 [details]
Patch

Fix windows build

[Joseph Pecoraro]

Comment on attachment 325613 [details]
Patch

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

r- for some more tests of just console.record/recordEnd generically.

What do you expect the nesting / interleaving support should be? Should we just try to do the simplest approach (1 recording at a time) or handle nested cases?

    console.record(canvas, {name: "outer"});
    console.record(canvas, {name: "inner"});
    ...
    console.recordEnd(canvas, {name: "inner"});
    console.recordEnd(canvas, {name: "outer"});

I'm fine with the simplest approach. I think profile() roughly keeps a stack of recordings because it translates to a single global timeline recording. But it does seem like overkill to track multiple recordings of the same thing at the same time.

> Source/JavaScriptCore/inspector/JSGlobalObjectConsoleClient.cpp:175
> +void JSGlobalObjectConsoleClient::record(ExecState*, Ref<ScriptArguments>&&)
> +{
> +    warnUnimplemented("console.record");
> +}
> +void JSGlobalObjectConsoleClient::recordEnd(ExecState*, Ref<ScriptArguments>&&)
> +{
> +    warnUnimplemented("console.recordEnd");
> +}

I think its fine to do nothing here. record/recordEnd just don't do anything yet inside of a JSContext.

Perhaps we could use them in the future for something.

Like if Threads become native? `console.record(thread);`? Not really sure what it would do.

In any case, I think an empty implementation, without a warning, would be acceptable.

> Source/WebCore/inspector/InspectorCanvas.cpp:103
> +    m_recordingName = emptyString();

A null string is simpler. You can continue to use "isEmpty" with the null string. You can get that with either of these:

    m_recordingName = { };
    m_recordingName = String();

The top one seems to be what people have been doing recently.

> Source/WebCore/inspector/InspectorInstrumentation.h:254
> +    static void consoleStartRecordingCanvas(HTMLCanvasElement&, std::optional<String> name = std::nullopt, std::optional<bool> singleFrame = std::nullopt, std::optional<int> memoryLimit = std::nullopt);

I think this should be grouped up by line 218-220 with the other console API methods and not the canvas methods.

I also don't think we want default values for these parameters. At least currently you always fill them in, so a default value might just lead to accidents.

> Source/WebCore/inspector/InspectorInstrumentation.h:428
> +    static void consoleStartRecordingCanvasImpl(InstrumentingAgents&, HTMLCanvasElement&, std::optional<String> name = std::nullopt, std::optional<bool> singleFrame = std::nullopt, std::optional<int> memoryLimit = std::nullopt);

Ditto.

> Source/WebCore/page/PageConsoleClient.cpp:226
> +    const JSC::JSValue& firstArgument = arguments->argumentAt(0).jsValue();

Maybe call this `targetValue` instead of `firstArgument`.

> Source/WebCore/page/PageConsoleClient.cpp:248
> +        const JSC::JSValue& secondArgument = arguments->argumentAt(1).jsValue();

Maybe call this `optionsValue` instead of `secondArgument`.

> Source/WebCore/page/PageConsoleClient.cpp:260
> +    if (canvas)
> +        InspectorInstrumentation::consoleStartRecordingCanvas(*canvas, name, singleFrame, memoryLimit);

If we expand `console.record(target, options)` to other different kinds of targets, I think we'll want to have a structure more like:

    JSObject* target = arguments->argumentAt(0).jsValue().getObject();
    JSObject* options = arguments->argumentCount() >= 2 ? arguments->argumentAt(1).jsValue().getObject() : nullptr;
    if (!target)
        return;

    if (HTMLCanvasElement* canvasElement = JSHTMLCanvasElement::toWrapped(vm, object)) {
        consoleRecordCanvas(*canvasElement, options);
        return;
    }

    if (CanvasRenderingContext2D* context2d = JSCanvasRenderingContext2D::toWrapped(vm, object)) {
        consoleRecordCanvas(context2d->canvas(), options);
        return;
    }

#if ENABLE(WEBGL)
    if (WebGLRenderingContext* contextWebGL = JSWebGLRenderingContext::toWrapped(vm, object))
        consoleRecordCanvas(contextWebGL->canvas(), options);
        return;
    }
#endif

    if (...something else...) {
        consoleRecordSomethingElse(..., options);
        return;
    }

Or an if/else chain to avoid a lot of extra lines for braces and returns:

    if (HTMLCanvasElement* canvasElement = JSHTMLCanvasElement::toWrapped(vm, object)) {
        consoleRecordCanvas(*canvasElement, options);
    else if (CanvasRenderingContext2D* context2d = JSCanvasRenderingContext2D::toWrapped(vm, object))
        consoleRecordCanvas(context2d->canvas(), options);
#if ENABLE(WEBGL)
    else if (WebGLRenderingContext* contextWebGL = JSWebGLRenderingContext::toWrapped(vm, object))
        consoleRecordCanvas(contextWebGL->canvas(), options);
#endif


This dramatically simplifies adding a new type, and parsing just the options relevant to that type.

> Source/WebCore/workers/WorkerConsoleClient.cpp:78
> +void WorkerConsoleClient::record(JSC::ExecState*, Ref<ScriptArguments>&&) { };
> +void WorkerConsoleClient::recordEnd(JSC::ExecState*, Ref<ScriptArguments>&&) { };

Style: Bad semicolons.

> Source/WebInspectorUI/UserInterface/Models/NativeFunctionParameters.js:177
> +        record: "object, [options]",
> +        recordEnd: "object",

Nice!

> LayoutTests/inspector/canvas/recording-webgl.html:550
> +        },

Style: Hmm, we don't normally have a comma after the test function. We kinda expect it to always be last.

> LayoutTests/js/console-expected.txt:166
> +console.record

Nice. We should have general fuzz tests for console.record and console.recordEnd.

    console.record();
    console.record(1);
    console.record(null);
    console.record([]);
    console.record("test");
    console.record(window);
    console.record(console);

    console.recordEnd();
    console.recordEnd(1);
    console.recordEnd([]);
    console.recordEnd(null);
    console.recordEnd("test");
    console.recordEnd(window);
    console.recordEnd(console);

    // Options
    console.record({}, 1);
    console.record({}, []);
    console.record({}, null);
    console.record({}, "test");
    console.record({}, window);
    console.record({}, console);

As well as a test to console.recordEnd on a canvas that isn't recording.

    console.recordEnd(canvas);
    console.recordEnd(canvas);
    console.recordEnd(canvas);

[Devin Rousso]

Submitted to the Console API GitHub standard <https://github.com/whatwg/console/issues/120>.

[Devin Rousso]

Comment on attachment 325613 [details]
Patch

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

> r- for some more tests of just console.record/recordEnd generically.
How should I test the functions if they have no return value or thrown errors?  I'm fine with adding tests, but I guess i'm just not sure how.

> What do you expect the nesting / interleaving support should be? Should we
> just try to do the simplest approach (1 recording at a time) or handle
> nested cases?
> 
>     console.record(canvas, {name: "outer"});
>     console.record(canvas, {name: "inner"});
>     ...
>     console.recordEnd(canvas, {name: "inner"});
>     console.recordEnd(canvas, {name: "outer"});
> 
> I'm fine with the simplest approach. I think profile() roughly keeps a stack
> of recordings because it translates to a single global timeline recording.
> But it does seem like overkill to track multiple recordings of the same
> thing at the same time.
The current functionality is such that each context can only have one recording at a time.  This was implemented from the very beginning and is also a limitation of the frontend UI as well.  I think we should keep it this way, but I also think that it wouldn't be that difficult to allow multiple recordings.

>> Source/WebCore/page/PageConsoleClient.cpp:260
>> +        InspectorInstrumentation::consoleStartRecordingCanvas(*canvas, name, singleFrame, memoryLimit);
> 
> If we expand `console.record(target, options)` to other different kinds of targets, I think we'll want to have a structure more like:
> 
>     JSObject* target = arguments->argumentAt(0).jsValue().getObject();
>     JSObject* options = arguments->argumentCount() >= 2 ? arguments->argumentAt(1).jsValue().getObject() : nullptr;
>     if (!target)
>         return;
> 
>     if (HTMLCanvasElement* canvasElement = JSHTMLCanvasElement::toWrapped(vm, object)) {
>         consoleRecordCanvas(*canvasElement, options);
>         return;
>     }
> 
>     if (CanvasRenderingContext2D* context2d = JSCanvasRenderingContext2D::toWrapped(vm, object)) {
>         consoleRecordCanvas(context2d->canvas(), options);
>         return;
>     }
> 
> #if ENABLE(WEBGL)
>     if (WebGLRenderingContext* contextWebGL = JSWebGLRenderingContext::toWrapped(vm, object))
>         consoleRecordCanvas(contextWebGL->canvas(), options);
>         return;
>     }
> #endif
> 
>     if (...something else...) {
>         consoleRecordSomethingElse(..., options);
>         return;
>     }
> 
> Or an if/else chain to avoid a lot of extra lines for braces and returns:
> 
>     if (HTMLCanvasElement* canvasElement = JSHTMLCanvasElement::toWrapped(vm, object)) {
>         consoleRecordCanvas(*canvasElement, options);
>     else if (CanvasRenderingContext2D* context2d = JSCanvasRenderingContext2D::toWrapped(vm, object))
>         consoleRecordCanvas(context2d->canvas(), options);
> #if ENABLE(WEBGL)
>     else if (WebGLRenderingContext* contextWebGL = JSWebGLRenderingContext::toWrapped(vm, object))
>         consoleRecordCanvas(contextWebGL->canvas(), options);
> #endif
> 
> 
> This dramatically simplifies adding a new type, and parsing just the options relevant to that type.

Good call.  That does read quite a bit nicer.  It's slightly annoying that in order to pull values out of a JSObject we need an ExecState, but I guess that's not a big deal :P

[Devin Rousso]

Created attachment 325642 [details]
Patch

[Devin Rousso]

Created attachment 325680 [details]
Patch

Add fuzzing test

[Devin Rousso]

Created attachment 325840 [details]
Patch

Rebase

[Timothy Hatcher]

I agree with Maciej. record() and recordEnd() are too generically named for this.

Some alt name ideas in roughly my order of preference:

* startRecordingCanvasDrawing() / stop…
* startCanvasRecording() / stop…
* recordCanvasDrawing() / stopRecordingCanvasDrawing()
* profileCanvas() / profileCanvasEnd()
* canvasStart() / canvasEnd()

[Timothy Hatcher]

I also see you have brought this up on the console standard GitHub. For reference that issue is: https://github.com/whatwg/console/issues/120

[Timothy Hatcher]

(In reply to Timothy Hatcher from comment #27)
> I also see you have brought this up on the console standard GitHub. For
> reference that issue is: https://github.com/whatwg/console/issues/120

And now I see Devin already mentioned this above.

I would be fine with record() and recordEnd() if it took a context object arg as proposed in the Github issue.

[Joseph Pecoraro]

Comment on attachment 325840 [details]
Patch

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

> Source/WebCore/inspector/InspectorInstrumentation.h:391
> +    static void consoleStartRecordingCanvasImpl(InstrumentingAgents&, HTMLCanvasElement&, JSC::ExecState*, JSC::JSObject* options);

Nit: ExecState&. If this is being called via `console` then we should be (and are) guaranteed an ExecState. This can drop the if check later.

[Devin Rousso]

Created attachment 328082 [details]
Patch

Rebase

[EWS Watchlist]

Comment on attachment 328082 [details]
Patch

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

New failing tests:
inspector/canvas/recording-webgl.html

[EWS Watchlist]

Created attachment 328085 [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

[EWS Watchlist]

Comment on attachment 328082 [details]
Patch

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

New failing tests:
inspector/canvas/recording-webgl.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 328082 [details]
Patch

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

New failing tests:
inspector/canvas/recording-webgl.html

[EWS Watchlist]

Created attachment 328087 [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 328181 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 328181 [details]
Patch

r=me, thanks for iterating on this.

[WebKit Commit Bot]

Comment on attachment 328181 [details]
Patch

Clearing flags on attachment: 328181

Committed r225488: <https://trac.webkit.org/changeset/225488>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.