Bug 175166 - Web Inspector: provide method for recording CanvasRenderingContext2D from JavaScript
Summary: Web Inspector: provide method for recording CanvasRenderingContext2D from Jav...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: WebInspectorCanvasRecording
  Show dependency treegraph
 
Reported: 2017-08-03 17:14 PDT by Devin Rousso
Modified: 2017-12-04 13:41 PST (History)
14 users (show)

See Also:


Attachments
Patch (28.13 KB, patch)
2017-08-04 00:11 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (31.29 KB, patch)
2017-10-28 21:19 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (1.06 MB, application/zip)
2017-10-28 22:17 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.46 MB, application/zip)
2017-10-28 22:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (1.84 MB, application/zip)
2017-10-28 22:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (14.74 MB, application/zip)
2017-10-28 22:58 PDT, Build Bot
no flags Details
Patch (36.74 KB, patch)
2017-10-31 13:42 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (1.05 MB, application/zip)
2017-10-31 14:42 PDT, Build Bot
no flags Details
Patch (36.98 KB, patch)
2017-10-31 14:43 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (37.00 KB, patch)
2017-11-01 12:19 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (37.09 KB, patch)
2017-11-01 15:39 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (40.52 KB, patch)
2017-11-01 22:06 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (40.91 KB, patch)
2017-11-02 21:23 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (40.82 KB, patch)
2017-12-01 03:22 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (2.25 MB, application/zip)
2017-12-01 04:22 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (2.63 MB, application/zip)
2017-12-01 04:33 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (2.96 MB, application/zip)
2017-12-01 04:50 PST, Build Bot
no flags Details
Patch (40.95 KB, patch)
2017-12-01 16:20 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2017-08-03 17:14:20 PDT
This would follow a similar process to console.time()/console.timeEnd().

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

Currently, it would
Comment 1 Devin Rousso 2017-08-03 17:15:20 PDT
(In reply to Devin Rousso from comment #0)
> Currently, it would

Oops. :P
Comment 2 Devin Rousso 2017-08-04 00:11:38 PDT
Created attachment 317219 [details]
Patch
Comment 3 Brian Burg 2017-08-04 11:51:34 PDT
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.
Comment 4 Radar WebKit Bug Importer 2017-08-23 12:11:18 PDT
<rdar://problem/34040740>
Comment 5 Maciej Stachowiak 2017-09-05 12:44:33 PDT
(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.
Comment 6 Devin Rousso 2017-10-28 21:19:48 PDT
Created attachment 325277 [details]
Patch
Comment 7 Build Bot 2017-10-28 22:17:15 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2017-10-28 22:17:17 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2017-10-28 22:31:49 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2017-10-28 22:31:51 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2017-10-28 22:41:05 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2017-10-28 22:41:11 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2017-10-28 22:58:56 PDT Comment hidden (obsolete)
Comment 14 Build Bot 2017-10-28 22:58:58 PDT Comment hidden (obsolete)
Comment 15 Devin Rousso 2017-10-31 13:42:21 PDT
Created attachment 325484 [details]
Patch

Attempt for bots.
Comment 16 Build Bot 2017-10-31 14:42:28 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2017-10-31 14:42:29 PDT Comment hidden (obsolete)
Comment 18 Devin Rousso 2017-10-31 14:43:44 PDT
Created attachment 325500 [details]
Patch

Gotta love spelling errors ᕕ( ᐛ )ᕗ
Comment 19 Devin Rousso 2017-11-01 12:19:51 PDT
Created attachment 325613 [details]
Patch

Fix windows build
Comment 20 Joseph Pecoraro 2017-11-01 14:47:03 PDT
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);
Comment 21 Devin Rousso 2017-11-01 14:59:38 PDT
Submitted to the Console API GitHub standard <https://github.com/whatwg/console/issues/120>.
Comment 22 Devin Rousso 2017-11-01 15:25:08 PDT
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
Comment 23 Devin Rousso 2017-11-01 15:39:14 PDT
Created attachment 325642 [details]
Patch
Comment 24 Devin Rousso 2017-11-01 22:06:33 PDT
Created attachment 325680 [details]
Patch

Add fuzzing test
Comment 25 Devin Rousso 2017-11-02 21:23:32 PDT
Created attachment 325840 [details]
Patch

Rebase
Comment 26 Timothy Hatcher 2017-11-30 09:03:52 PST
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()
Comment 27 Timothy Hatcher 2017-11-30 09:06:22 PST
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
Comment 28 Timothy Hatcher 2017-11-30 09:15:22 PST
(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.
Comment 29 Joseph Pecoraro 2017-11-30 11:14:57 PST
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.
Comment 30 Devin Rousso 2017-12-01 03:22:30 PST
Created attachment 328082 [details]
Patch

Rebase
Comment 31 Build Bot 2017-12-01 04:22:48 PST Comment hidden (obsolete)
Comment 32 Build Bot 2017-12-01 04:22:49 PST Comment hidden (obsolete)
Comment 33 Build Bot 2017-12-01 04:33:49 PST Comment hidden (obsolete)
Comment 34 Build Bot 2017-12-01 04:33:51 PST Comment hidden (obsolete)
Comment 35 Build Bot 2017-12-01 04:50:46 PST Comment hidden (obsolete)
Comment 36 Build Bot 2017-12-01 04:50:48 PST Comment hidden (obsolete)
Comment 37 Devin Rousso 2017-12-01 16:20:05 PST
Created attachment 328181 [details]
Patch
Comment 38 Joseph Pecoraro 2017-12-04 12:52:09 PST
Comment on attachment 328181 [details]
Patch

r=me, thanks for iterating on this.
Comment 39 WebKit Commit Bot 2017-12-04 13:40:59 PST
Comment on attachment 328181 [details]
Patch

Clearing flags on attachment: 328181

Committed r225488: <https://trac.webkit.org/changeset/225488>
Comment 40 WebKit Commit Bot 2017-12-04 13:41:01 PST
All reviewed patches have been landed.  Closing bug.