Bug 190856 – Web Inspector: Canvas: create a setting for auto-recording newly created contexts

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

This is especially useful for situations where double-buffering is used with a temporary canvas (e.g. `WI.Popover.prototype._drawBackground`).  Ideally, there would be a way to configure how long the recording should be (e.g. single vs multiple frames) when enabling this setting.

[Devin Rousso]

*** Bug 190855 has been marked as a duplicate of this bug. ***

[Devin Rousso]

Created attachment 353261 [details]
Patch

Not a huge fan of including an <input> in the navigation bar, but I'm not sure how else to do it 🤔

[Devin Rousso]

Created attachment 353262 [details]
[Video] After Patch is applied

[Devin Rousso]

Created attachment 353263 [details]
[Image] After Patch is applied

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

Created attachment 353267 [details]
Patch

Small refactoring for systems that don't support WebGL

[Blaze Burg]

Comment on attachment 353267 [details]
Patch

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

Overall I like this patch. However, the wording in the navigation bar seems wordy. How about

[X] Automatically Capture <1> Frames

Where having an unchecked checkbox means frame limit=0, and checked with any other value sets the frame cap. The toggle makes this easy to toggle on and off without editing to zero and back. It also avoids the implicit knowledge in the current design that setting this to '0' will turn off the feature.

> Source/JavaScriptCore/inspector/protocol/Canvas.json:120
> +                { "name": "memoryLimit", "type": "integer", "optional": true, "description": "Memory limit of recorded data (100MB wgeb bit specified)." }

Typo: (100MB when not specified)

> Source/WebCore/inspector/InspectorCanvas.cpp:108
> +    m_frameLimit = static_cast<size_t>(-1); // Unlimited

I really don't like this magic value. Can it use an optional?

> Source/WebInspectorUI/UserInterface/Base/Setting.js:117
> +    creationRecordingFrameCount: new WI.Setting("creation-recording-frame-count", 1),

This name really bugs me. Can it be something like "autoCaptureNewCanvasesWithFrameLimit"

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:46
> +        return window.CanvasAgent && CanvasAgent.setCreationRecordingFrameCount;

Ditto

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:98
> +        CanvasAgent.setCreationRecordingFrameCount(frameCount, (error) => {

Use a promise?

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:290
> +        // COMPATIBILITY (iOS 12.0): `frameCount` did not exist yet

Nit: 12.1
Nit: .

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:347
> +        else if (initiator === WI.Recording.Initiator.Creation)

AutoCapture?

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:-176
> -.popover-content > .tree-outline .item.recording > .icon {

What happened to these rules?

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:64
> +                creationRecordingFrameCountInput.style.setProperty("width", ((creationRecordingFrameCountInput.value.length * 7) + 21) + "px");

This seems kind of obscure? We can't assume the glyph will always be 7px wide, it may be different i.e., when using Arabic script.

[Blaze Burg]

Comment on attachment 353267 [details]
Patch

r- until we have consensus on the UI

[Devin Rousso]

Comment on attachment 353267 [details]
Patch

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

(In reply to Brian Burg from comment #7)
> Overall I like this patch. However, the wording in the navigation bar seems
> wordy. How about
> 
> [X] Automatically Capture <1> Frames
> 
> Where having an unchecked checkbox means frame limit=0, and checked with any
> other value sets the frame cap. The toggle makes this easy to toggle on and
> off without editing to zero and back. It also avoids the implicit knowledge
> in the current design that setting this to '0' will turn off the feature.
I originally had something like this, but it seemed too "busy".  I'll give it another shot and post a screenshot.

>> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:-176
>> -.popover-content > .tree-outline .item.recording > .icon {
> 
> What happened to these rules?

They aren't used by anything.  We don't create popovers for Canvas (except for `WI.InlineSwatch`s).

>> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:64
>> +                creationRecordingFrameCountInput.style.setProperty("width", ((creationRecordingFrameCountInput.value.length * 7) + 21) + "px");
> 
> This seems kind of obscure? We can't assume the glyph will always be 7px wide, it may be different i.e., when using Arabic script.

Good point.  I'll play around and see if I can get `em` units to work.

[Devin Rousso]

Created attachment 353446 [details]
Patch

[Devin Rousso]

Created attachment 353447 [details]
[Image] After Patch is applied

[EWS Watchlist]

Comment on attachment 353446 [details]
Patch

Attachment 353446 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9787453

New failing tests:
http/wpt/css/css-animations/start-animation-001.html

[EWS Watchlist]

Created attachment 353449 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Blaze Burg]

Comment on attachment 353446 [details]
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

[Devin Rousso]

(In reply to Brian Burg from comment #14)
> 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.
I am somewhat against the idea of not having the up/down arrows, as that's a pretty standard thing we do throughout WebInspector (and the system AFAIK).  I'll give it a shot.

[Devin Rousso]

Created attachment 353562 [details]
Patch

[Devin Rousso]

Created attachment 353567 [details]
Patch

Rebase

[Devin Rousso]

Comment on attachment 353567 [details]
Patch

Oops.

[Devin Rousso]

Comment on attachment 353567 [details]
Patch

Oh wait no this has been reviewed :P

[WebKit Commit Bot]

Comment on attachment 353567 [details]
Patch

Clearing flags on attachment: 353567

Committed r237670: <https://trac.webkit.org/changeset/237670>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45720965>

[Truitt Savell]

The new test inspector/canvas/setRecordingAutoCaptureFrameCount.html

added in https://trac.webkit.org/changeset/237670/webkit

is failing off the bat. 

History: 
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fcanvas%2FsetRecordingAutoCaptureFrameCount.html

Diff:
--- /Volumes/Data/slave/highsierra-debug-tests-wk1/build/layout-test-results/inspector/canvas/setRecordingAutoCaptureFrameCount-expected.txt
+++ /Volumes/Data/slave/highsierra-debug-tests-wk1/build/layout-test-results/inspector/canvas/setRecordingAutoCaptureFrameCount-actual.txt
@@ -26,6 +26,7 @@
 PASS: Canvas should have one finished recording.
 
 -- Running test case: Canvas.setRecordingAutoCaptureFrameCount.WebGL.Multiple
+PASS: Recording started.
 PASS: Canvas should have no finished recordings.
 PASS: Canvas should be actively recording.
 PASS: Recording should have 2 frames.

[Devin Rousso]

(In reply to Truitt Savell from comment #23)
> The new test inspector/canvas/setRecordingAutoCaptureFrameCount.html
> 
> added in https://trac.webkit.org/changeset/237670/webkit
> 
> is failing off the bat. 
> 
> History: 
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=inspector%2Fcanvas%2FsetRecordingAutoCaptureFrame
> Count.html
> 
> Diff:
> ---
> /Volumes/Data/slave/highsierra-debug-tests-wk1/build/layout-test-results/
> inspector/canvas/setRecordingAutoCaptureFrameCount-expected.txt
> +++
> /Volumes/Data/slave/highsierra-debug-tests-wk1/build/layout-test-results/
> inspector/canvas/setRecordingAutoCaptureFrameCount-actual.txt
> @@ -26,6 +26,7 @@
>  PASS: Canvas should have one finished recording.
>  
>  -- Running test case:
> Canvas.setRecordingAutoCaptureFrameCount.WebGL.Multiple
> +PASS: Recording started.
>  PASS: Canvas should have no finished recordings.
>  PASS: Canvas should be actively recording.
>  PASS: Recording should have 2 frames.
Landed fix <https://trac.webkit.org/changeset/237681/webkit>