[webkit-reviews] review granted: [Bug 190856] Web Inspector: Canvas: create a setting for auto-recording newly created contexts : [Attachment 353446] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 190856: Web Inspector: Canvas: create a setting for auto-recording newly
created contexts
https://bugs.webkit.org/show_bug.cgi?id=190856

Attachment 353446: Patch

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




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

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

r=me on code changes, but I'd like you to try out the following suggestion.

I see what you mean about it looking too busy. However, I think it would look
better without the up/down arrows. The non-uniform text spacing induced by the
<input> styling also is jarring, but that can be tweaked easily. I think most
users are going to be able to figure out that focusing the number and pressing
up/down will adjust the value accordingly.

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:289
> +	   recordingOptions.frameCount = *frameCount;

When using optionals I prefer .value() as it's less ambiguous than dereference
operator.

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:56
> +	       String.format(WI.UIString("Record %s frames after creation"),
[this._recordingAutoCaptureFrameCountInputElement], String.standardFormatters,
label, (a, b) => {

Alternate label:

Capture first %s frames