[webkit-reviews] review denied: [Bug 178185] Web Inspector: Canvas tab: show detailed status during canvas recording : [Attachment 326378] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Devin Rousso <webkit at devinrousso.com>'s
request for review:
Bug 178185: Web Inspector: Canvas tab: show detailed status during canvas
recording
https://bugs.webkit.org/show_bug.cgi?id=178185

Attachment 326378: Patch

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




--- Comment #12 from Brian Burg <bburg at apple.com> ---
Comment on attachment 326378
  --> https://bugs.webkit.org/attachment.cgi?id=326378
Patch

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

r- for now but overall it looks good. I'd like some clarity on what bufferUsed
means, and see that it's exercised in the updated tests if it is deterministic.

> Source/WebCore/ChangeLog:13
> +	   After each recorded frame, fire a progress event with the recorded
frames as data. This will

If this event fires for every frame, why does it include an array of frames? I
prefer the array approach since it lets us use a different coalescing strategy
without changing the protocol or frontend. But maybe we should adjust this
comment and say something like this in the protocol documentation.

> Source/JavaScriptCore/inspector/protocol/Canvas.json:191
> +		   { "name": "bufferUsed", "type": "integer" }

This needs explanation. What is the unit of measure? is it the amount used by
the reported frames, the amount used by the backend before/after releasing the
frames, ??

> Source/WebCore/inspector/InspectorCanvas.cpp:118
> +    return m_frames;

Please write this as !!m_frames for clarity

> Source/WebCore/inspector/InspectorCanvas.h:65
> +    RefPtr<Inspector::Protocol::Recording::InitialState>&&
releaseInitialState() WARN_UNUSED_RETURN;

What does this do?

> Source/WebCore/inspector/InspectorCanvas.h:74
> +    long bufferUsed() const { return m_bufferUsed; }

Ditto the above comment in the change log. This could use a better name, like
bufferCapacityInBytes or whatever the semantics are.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:163
> +	       bufferUsed,

Ditto.

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:124
> +	       if (context.getImageData)

Pardon the ignorance but are these different cases intended for 2d vs webgl?
Would it be clearer to switch on the context type? And then what is the last
fallback for.

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:139
> +    height: calc(var(--preview-height) - 1em - (2 *
var(--progress-padding)));

Wow. Yo might want to translate this into English.

> LayoutTests/inspector/canvas/resources/recording-utilities.js:109
> +	   let frameCount = 0;

Shouldn't we be checking the size of the frames somewhere (bufferUsed)? Or is
this not deterministic enough to put in a test?