[webkit-reviews] review granted: [Bug 174483] Web Inspector: Record actions performed on WebGLRenderingContext : [Attachment 318945] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has granted Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 174483: Web Inspector: Record actions performed on WebGLRenderingContext
https://bugs.webkit.org/show_bug.cgi?id=174483

Attachment 318945: Patch

https://bugs.webkit.org/attachment.cgi?id=318945&action=review




--- Comment #23 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 318945
  --> https://bugs.webkit.org/attachment.cgi?id=318945
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.