Bug 175166

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

Created attachment 317219 [details] Patch

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.

<rdar://problem/34040740>

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

Created attachment 325277 [details] Patch

Created attachment 325484 [details] Patch Attempt for bots.

Created attachment 325500 [details] Patch Gotta love spelling errors ᕕ( ᐛ )ᕗ

Created attachment 325613 [details] Patch Fix windows build

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

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

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

Created attachment 325642 [details] Patch

Created attachment 325680 [details] Patch Add fuzzing test

Created attachment 325840 [details] Patch Rebase

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

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

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

Created attachment 328082 [details] Patch Rebase

Created attachment 328181 [details] Patch

Comment on attachment 328181 [details] Patch r=me, thanks for iterating on this.

Comment on attachment 328181 [details] Patch Clearing flags on attachment: 328181 Committed r225488: <https://trac.webkit.org/changeset/225488>

All reviewed patches have been landed. Closing bug.