Bug 190856

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

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 🤔

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

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

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

Created attachment 353267 [details] Patch Small refactoring for systems that don't support WebGL

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 on attachment 353267 [details] Patch r- until we have consensus on the UI

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.

Created attachment 353446 [details] Patch

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

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

(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.

Created attachment 353562 [details] Patch

Created attachment 353567 [details] Patch Rebase

Comment on attachment 353567 [details] Patch Oops.

Comment on attachment 353567 [details] Patch Oh wait no this has been reviewed :P

Comment on attachment 353567 [details] Patch Clearing flags on attachment: 353567 Committed r237670: <https://trac.webkit.org/changeset/237670>

All reviewed patches have been landed. Closing bug.

<rdar://problem/45720965>

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.

(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>