Bug 190856

Summary: Web Inspector: Canvas: create a setting for auto-recording newly created contexts
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, rniwa, saam, tsavell, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 173807, 175485, 191553, 192189, 192454    
Attachments:
Description Flags
Patch
none
[Video] After Patch is applied
none
[Image] After Patch is applied
none
Patch
none
Patch
none
[Image] After Patch is applied
none
Archive of layout-test-results from ews101 for mac-sierra
none
Patch
none
Patch none

Description Devin Rousso 2018-10-23 18:41:42 PDT
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.
Comment 1 Devin Rousso 2018-10-23 22:24:27 PDT
*** Bug 190855 has been marked as a duplicate of this bug. ***
Comment 2 Devin Rousso 2018-10-28 14:05:02 PDT
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 🤔
Comment 3 Devin Rousso 2018-10-28 14:05:48 PDT
Created attachment 353262 [details]
[Video] After Patch is applied
Comment 4 Devin Rousso 2018-10-28 14:06:30 PDT
Created attachment 353263 [details]
[Image] After Patch is applied
Comment 5 EWS Watchlist 2018-10-28 14:06:54 PDT
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 6 Devin Rousso 2018-10-28 16:25:32 PDT
Created attachment 353267 [details]
Patch

Small refactoring for systems that don't support WebGL
Comment 7 BJ Burg 2018-10-30 14:00:57 PDT
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.
Comment 8 BJ Burg 2018-10-30 14:19:51 PDT
Comment on attachment 353267 [details]
Patch

r- until we have consensus on the UI
Comment 9 Devin Rousso 2018-10-30 14:41:07 PDT
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.
Comment 10 Devin Rousso 2018-10-30 17:37:56 PDT
Created attachment 353446 [details]
Patch
Comment 11 Devin Rousso 2018-10-30 17:39:22 PDT
Created attachment 353447 [details]
[Image] After Patch is applied
Comment 12 EWS Watchlist 2018-10-30 18:25:29 PDT Comment hidden (obsolete)
Comment 13 EWS Watchlist 2018-10-30 18:25:30 PDT Comment hidden (obsolete)
Comment 14 BJ Burg 2018-10-31 09:18:24 PDT
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
Comment 15 Devin Rousso 2018-10-31 11:31:57 PDT
(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.
Comment 16 Devin Rousso 2018-10-31 17:53:13 PDT
Created attachment 353562 [details]
Patch
Comment 17 Devin Rousso 2018-10-31 18:19:26 PDT
Created attachment 353567 [details]
Patch

Rebase
Comment 18 Devin Rousso 2018-10-31 20:47:27 PDT
Comment on attachment 353567 [details]
Patch

Oops.
Comment 19 Devin Rousso 2018-10-31 20:47:51 PDT
Comment on attachment 353567 [details]
Patch

Oh wait no this has been reviewed :P
Comment 20 WebKit Commit Bot 2018-10-31 21:13:51 PDT
Comment on attachment 353567 [details]
Patch

Clearing flags on attachment: 353567

Committed r237670: <https://trac.webkit.org/changeset/237670>
Comment 21 WebKit Commit Bot 2018-10-31 21:13:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Radar WebKit Bug Importer 2018-10-31 21:14:37 PDT
<rdar://problem/45720965>
Comment 23 Truitt Savell 2018-11-01 08:28:44 PDT
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.
Comment 24 Devin Rousso 2018-11-01 09:13:33 PDT
(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>