Use the protocol/instrumentation logic created in <https://webkit.org/b/174481> Web Inspector: create protocol functions for recording Canvas contexts
Created attachment 317858 [details] Patch
This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)
Created attachment 317906 [details] Patch
Created attachment 317910 [details] Patch
Created attachment 317913 [details] Patch
Comment on attachment 317913 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317913&action=review Nice! There are so many types that are just Strings right now, but I think this makes sense and we can just tackle them as we go. > Source/WebCore/ChangeLog:18 > + Modify the generated code for `CallTracingCallback` so that optional/nullable types are > + dereferenced if they have a value. Might want to change "are dereferenced if" to "are only dereferenced if" > Source/WebCore/inspector/InspectorCanvas.cpp:135 > + if (is<WebGLRenderingContext>(m_canvas.renderingContext()) && (name == "clear" || name == "drawArrays" || name == "drawElements")) Lets extract a helper for the list of WebGL actions that require a snapshot: static bool webGLActionNeedSnapshot(const String& name) { return name == "clear" || name == "drawArrays" || name == "drawElements"; } > Source/WebCore/inspector/InspectorCanvas.cpp:150 > + if (m_actionNeedsSnapshot) { > + m_actionNeedsSnapshot->addItem(indexForData(getCanvasContent())); > + m_actionNeedsSnapshot = nullptr; > + } This happens in multiple places. It could be a method `appendActionSnapshotIfNeeded()`. > Source/WebCore/inspector/InspectorCanvas.cpp:506 > + [&] (ImageData* value) { parametersData->addItem(indexForData(value)); }, > +#if ENABLE(WEBGL) > + [&] (const WebGLBuffer*) { parametersData->addItem(indexForData(String("WebGLBuffer"))); }, > + [&] (const WebGLFramebuffer*) { parametersData->addItem(indexForData(String("WebGLFramebuffer"))); }, > + [&] (const WebGLProgram*) { parametersData->addItem(indexForData(String("WebGLProgram"))); }, > + [&] (const WebGLRenderbuffer*) { parametersData->addItem(indexForData(String("WebGLRenderbuffer"))); }, > + [&] (const WebGLShader*) { parametersData->addItem(indexForData(String("WebGLShader"))); }, > + [&] (const WebGLTexture*) { parametersData->addItem(indexForData(String("WebGLTexture"))); }, > + [&] (const WebGLUniformLocation*) { parametersData->addItem(indexForData(String("WebGLUniformLocation"))); }, > +#endif > + [&] (const RefPtr<ArrayBuffer>&) { parametersData->addItem(indexForData("BufferDataSource")); }, > + [&] (const RefPtr<ArrayBufferView>&) { parametersData->addItem(indexForData("BufferDataSource")); }, We should have a FIXME to handle many of these types later. For example we will want to be able to link to Programs/Shaders, View Texture/Buffer data, and UniformLocation seems like a small int/type tuple. > Source/WebCore/inspector/InspectorCanvas.h:82 > + String getCanvasContent(); How about "getCanvasContentAsDataURL" or "getCanvasContentAsPNGDataURL" > Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:696 > + "texImage2D", > + "texImage2D", Duplicate > Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:700 > + "texSubImage2D", > + "texSubImage2D", Duplicate > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:378 > + _generateContentCanvasWebGL(index, options = {}) So unlike 2D Canvas, here we are just showing an <img> with the last "snapshot" (image) from the last draw operation (clear, draw*) on the context? Clever! Will there be an opportunity to use the Show/Hide Path button on WebGL contexts? > LayoutTests/inspector/canvas/recording-webgl-expected.txt:398 > + "actions": [ > + [ > + 56, > + [ > + 1 > + ], > + [ > + 57, > + 5 > + ], > + 58 > + ] > + ] Yay, an action with a snapshot. Gosh I wonder if we could pretty print this ourselves so its easier to read. For example this would be wonderful given the format: { "actions": [ 56, [1], [57, 5], 58 ] } > LayoutTests/inspector/canvas/recording-webgl-expected.txt:833 > + "actions": [ > + [ > + 115, > + [ > + 1, > + 2, > + 3 > + ], > + [ > + 116, > + 5 > + ], > + 58 > + ] > + ] > + }, Okay, its unfortunate that this snapshot is the same one (58). Because the last was from clear() which means the WebGL context here is still clear. I think we will want to have a test specifically to show that snapshots progress: ctx.clear(); // produces an empty snapshot ... ctx.drawArrays(); // produces a new snapshot ... ctx.drawElements(); // produces a new snapshot ... ctx.clear(); // probably produces the empty snapshot
Comment on attachment 317913 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317913&action=review >> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:378 >> + _generateContentCanvasWebGL(index, options = {}) > > So unlike 2D Canvas, here we are just showing an <img> with the last "snapshot" (image) from the last draw operation (clear, draw*) on the context? Clever! > > Will there be an opportunity to use the Show/Hide Path button on WebGL contexts? The concept of Path doesn't exist for WebGL, so we don't show the NavigationItem. >> LayoutTests/inspector/canvas/recording-webgl-expected.txt:833 >> + }, > > Okay, its unfortunate that this snapshot is the same one (58). Because the last was from clear() which means the WebGL context here is still clear. > > I think we will want to have a test specifically to show that snapshots progress: > > ctx.clear(); // produces an empty snapshot > ... > ctx.drawArrays(); // produces a new snapshot > ... > ctx.drawElements(); // produces a new snapshot > ... > ctx.clear(); // probably produces the empty snapshot Good idea =D
Created attachment 318687 [details] Patch
Comment on attachment 318687 [details] Patch Attachment 318687 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4356525 New failing tests: inspector/canvas/recording-webgl-snapshots.html
Created attachment 318705 [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 318687 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318687&action=review > Source/WebCore/ChangeLog:35 > + object, as we don't need the value since the preview will be generated by the `toDataURL` Style: Comma splice. Easier to read as two sentences: For all non-primitive WebGL objects, send a string corresponding to the object's name. The value isn't needed since the preview will be generated by the `toDataURL` snapshots taken after every visual action. > Source/WebInspectorUI/ChangeLog:20 > + Include an optional 4th parameter `snapshot` that will be sent for visual actions when Style: 4th -> fourth. > Source/WebCore/inspector/InspectorCanvas.cpp:106 > +static bool webGLActionNeedSnapshot(const String& name) This function name is off. What do you think about `shouldSnapshotWebGLAction()`? > Source/WebCore/inspector/InspectorCanvas.h:112 > + RefPtr<Inspector::Protocol::Array<Inspector::InspectorValue>> m_actionNeedsSnapshot; This name sounds like a boolean, not an action. What about `m_actionNeedingSnapshot`? Would be more consistent with the rest of WebKit too (based on a case-sensitive search for "Needing" in WebCore). > Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:33 > + this._payloadSnapshot = snapshot || -1; I think `this._snapshotIndex` might be clearer. These are all passed as indices to Recording.swizzle, how about renaming them all? > Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:124 > + if (!isNaN(this._payloadSnapshot)) I don't think this will ever be NaN, based on RecordingAction's constructor. > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:37 > this._previewImageElement = null; Left off here. Will complete review later.
Comment on attachment 318687 [details] Patch Attachment 318687 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4356543 New failing tests: inspector/canvas/recording-webgl-snapshots.html
Created attachment 318707 [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 318687 [details] Patch Attachment 318687 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4356547 Number of test failures exceeded the failure limit.
Created attachment 318710 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 318687 [details] Patch Attachment 318687 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4356622 New failing tests: inspector/canvas/recording-webgl-snapshots.html
Created attachment 318713 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 318733 [details] Patch
Comment on attachment 318687 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318687&action=review >> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:33 >> + this._payloadSnapshot = snapshot || -1; > > I think `this._snapshotIndex` might be clearer. These are all passed as indices to Recording.swizzle, how about renaming them all? This only really applies to name and snapshot, as parameters and trace are both arrays. Furthermore, the items in parameters *might* be a swizzle index and they might also just be a regular number. I'd rather keep "payload" somewhere in the name to keep it clear that these values are from the JSON payload/import.
Created attachment 318793 [details] Patch
<rdar://problem/34040722>
Created attachment 318945 [details] Patch
Comment on attachment 318945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318945&action=review r=me, with nites/style stuff. > Source/WebCore/bindings/scripts/test/JS/JSTestCallTracer.cpp:428 > + UNUSED_PARAM(throwScope); Both of these are used below. > Source/WebCore/bindings/scripts/test/JS/JSTestCallTracer.cpp:452 > + UNUSED_PARAM(throwScope); Ditto. > Source/WebCore/bindings/scripts/test/JS/JSTestCallTracer.cpp:474 > + UNUSED_PARAM(throwScope); Ditto. > Source/WebCore/inspector/InspectorCanvas.cpp:447 > + WebGLRenderingContextBase* contextWebGLBase = downcast<WebGLRenderingContextBase>(canvasRenderingContext); Style: the return type is specified in the template parameter, so auto* would be fine here. > Source/WebInspectorUI/UserInterface/Models/Recording.js:173 > + // FIXME: regenerate the WebGL objects from the sent data instead of using the placeholder string. If you're still considering this enhancement, file a follow-up and add a link here. > Source/WebInspectorUI/UserInterface/Models/Recording.js:194 > + this._swizzle[index][type] = new ImageData(new Uint8ClampedArray(data[0]), parseInt(data[1]), parseInt(data[2])); Might be nice to break out the array accesses into consts, so we know what the ImageData parameters mean. > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:409 > + } else if (actions[visualIndex] instanceof WI.RecordingInitialStateAction) Should just be an `if`, since you return inside the branch above. > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:420 > + if (options.onCompleteCallback) I realize this was already in place from a previous patch, but generally we don't prefix event handlers with "on". What about `actionCompletedCallback`? It also better describes the purpose of the callback. > LayoutTests/inspector/canvas/recording-webgl-snapshots.html:119 > + CanvasAgent.requestRecording(canvases[0].identifier, singleFrame, (error) => { Neat, I didn't know we could record a single frame! > LayoutTests/inspector/canvas/recording-webgl.html:53 > +function ignoreException(f){ Variable `f` should have a better name. > LayoutTests/inspector/canvas/recording-webgl.html:70 > + let frames = [ Aren't these "actions"? > LayoutTests/inspector/canvas/recording-webgl.html:499 > + function f() { This function should have a better name.
Comment on attachment 318945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318945&action=review >> Source/WebCore/bindings/scripts/test/JS/JSTestCallTracer.cpp:428 >> + UNUSED_PARAM(throwScope); > > Both of these are used below. This is generated code. The generator probably generates this for simplicity since it compiles to nothing we probably do it to keep the generator simple. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:-474 > - // <https://webkit.org/b/174483> Web Inspector: Record actions performed on WebGLRenderingContext We should file a new bug for WebGL2. It seems that would be pretty similar to this but WebGL2RenderingContext.
Comment on attachment 318945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318945&action=review >> Source/WebInspectorUI/UserInterface/Models/Recording.js:194 >> + this._swizzle[index][type] = new ImageData(new Uint8ClampedArray(data[0]), parseInt(data[1]), parseInt(data[2])); > > Might be nice to break out the array accesses into consts, so we know what the ImageData parameters mean. I think that can be determined by what the parameters for ImageData expect. >> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:409 >> + } else if (actions[visualIndex] instanceof WI.RecordingInitialStateAction) > > Should just be an `if`, since you return inside the branch above. Oops. Good catch! >> LayoutTests/inspector/canvas/recording-webgl.html:70 >> + let frames = [ > > Aren't these "actions"? So each of these will be called in a separate requestAnimationFrame(). The idea is that every unique function/getter/setter gets its own frame so that it's easier to distinguish each grouping of recorded actions. This pattern follows what Canvas-2D uses.
Created attachment 319152 [details] Patch
Comment on attachment 319152 [details] Patch Clearing flags on attachment: 319152 Committed r221232: <http://trac.webkit.org/changeset/221232>
All reviewed patches have been landed. Closing bug.