Bug 180509

Summary: Web Inspector: Canvas Tab: Improve feedback after stopping a recording
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, rniwa, webkit-bug-importer
Priority: P1 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=182995
https://bugs.webkit.org/show_bug.cgi?id=185152
Bug Depends on: 178744    
Bug Blocks: 173807, 175485    
Attachments:
Description Flags
[Image] Multiple "Waiting for frames…" messages
none
Patch
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Patch bburg: review-

Matt Baker
Reported 2017-12-06 15:53:12 PST
Summary: Improve feedback after stopping a recording. For large recordings, there can be a substantial delay between the user stopping the recording, and the recording view appearing. We should give feedback that the recording is in fact loading. Steps to Reproduce: 1. Goto https://developer.mozilla.org/en-US/docs/Web/API/Canvas_API/Tutorial/Basic_animations 2. Scroll to the bottom of the inspected page 3. Open Canvas tab => All three canvases should be visible 4. Start recording the "stopwatch" canvas 5. Record a significant amount of data (> 3 MB) 6. Stop the recording => Recording UI stops updating, recording appears after a long delay As soon as the user stops the recording, we should show a "Preparing data..." message. Additionally, CanvasManager should wait until the Canvas.stopRecording round-trip completes before marking the recording finished.
Attachments
[Image] Multiple "Waiting for frames…" messages (860.30 KB, image/png)
2017-12-06 15:55 PST, Matt Baker 		no flags
Details
Patch (8.29 KB, patch)
2017-12-06 16:17 PST, Matt Baker 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews103 for mac-elcapitan (2.18 MB, application/zip)
2017-12-06 17:15 PST, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (3.40 MB, application/zip)
2017-12-06 17:18 PST, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews112 for mac-elcapitan (2.91 MB, application/zip)
2017-12-06 17:44 PST, EWS Watchlist 		no flags
Details
Patch (11.49 KB, patch)
2017-12-06 23:37 PST, Matt Baker 		bburg: review-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Matt Baker
Comment 1 2017-12-06 15:55:01 PST
Created attachment 328641 [details] [Image] Multiple "Waiting for frames…" messages This patch also address an issue where ALL canvases would update their UI when a recording begins.
Matt Baker
Comment 2 2017-12-06 16:17:47 PST
Created attachment 328646 [details] Patch
Radar WebKit Bug Importer
Comment 3 2017-12-06 16:18:11 PST
<rdar://problem/35895738>
EWS Watchlist
Comment 4 2017-12-06 17:15:00 PST
Comment on attachment 328646 [details] Patch Attachment 328646 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5522472 New failing tests: inspector/canvas/recording-2d.html
EWS Watchlist
Comment 5 2017-12-06 17:15:02 PST
Created attachment 328658 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
EWS Watchlist
Comment 6 2017-12-06 17:18:41 PST
Comment on attachment 328646 [details] Patch Attachment 328646 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5522428 New failing tests: inspector/canvas/recording-2d.html
EWS Watchlist
Comment 7 2017-12-06 17:18:42 PST
Created attachment 328659 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
EWS Watchlist
Comment 8 2017-12-06 17:44:48 PST
Comment on attachment 328646 [details] Patch Attachment 328646 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5522613 New failing tests: inspector/canvas/recording-2d.html
EWS Watchlist
Comment 9 2017-12-06 17:44:49 PST
Created attachment 328667 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Matt Baker
Comment 10 2017-12-06 23:37:31 PST
Created attachment 328684 [details] Patch
Blaze Burg
Comment 11 2017-12-08 13:49:39 PST
Comment on attachment 328684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328684&action=review The approach in this patch is prone to getting out of sync when recordings are started and stopped via the Console API. For that reason, I think you should revise this patch. I would prefer that we expose a 'state' property on the recording object and fire events when the state changes. This mean we can test the expected state transitions in a layout test rather than the canvas view trying to track model state. I think the states you need are Capturing, Loading, Ready. > LayoutTests/inspector/canvas/recording-2d.html:505 > + WI.canvasManager.startRecording(canvas); Nit: this test would be a lot simpler to follow if these methods returned promises and the test case turned into an async function.
Devin Rousso
Comment 12 2017-12-13 18:01:57 PST
Comment on attachment 328684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328684&action=review > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:391 > + this._recordingProgressElement = null; Is it necessary to delete the element after recording? I think our style is to not delete elements if they are saved to a member variable. I could go either way on this though. >> LayoutTests/inspector/canvas/recording-2d.html:505 >> + WI.canvasManager.startRecording(canvas); > > Nit: this test would be a lot simpler to follow if these methods returned promises and the test case turned into an async function. This is usually why I prefer using the Agent calls instead of the manager, as it provides callback and promise support. In this case, however, the point is to test the manager :P
Matt Baker
Comment 13 2017-12-15 15:33:59 PST
(In reply to Brian Burg from comment #11) > Comment on attachment 328684 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=328684&action=review > > The approach in this patch is prone to getting out of sync when recordings > are started and stopped via the Console API. For that reason, I think you > should revise this patch. CanvasManager can keep its internal state consistent by flagging a canvas as recording once the first progress message is received. Will change. > I would prefer that we expose a 'state' property on the recording object and > fire events when the state changes. This mean we can test the expected state > transitions in a layout test rather than the canvas view trying to track > model state. I think the states you need are Capturing, Loading, Ready. The problem with this approach is that the recording object doesn't exist until the canvas recording finishes. This could still work, but it would need to be done at the CanvasManager level. > > LayoutTests/inspector/canvas/recording-2d.html:505 > > + WI.canvasManager.startRecording(canvas); > > Nit: this test would be a lot simpler to follow if these methods returned > promises and the test case turned into an async function.
Matt Baker
Comment 14 2017-12-15 16:24:14 PST
Actually I think it will be a lot more straightforward to just have CanvasAgent dispatch RecordingStarted events, and include a `fromConsole` parameter, rather than having the UI synthesize events and infer the origin of the recording.
Devin Rousso
Comment 15 2017-12-15 20:23:01 PST
(In reply to Matt Baker from comment #14) > Actually I think it will be a lot more straightforward to just have > CanvasAgent dispatch RecordingStarted events, and include a `fromConsole` > parameter, rather than having the UI synthesize events and infer the origin > of the recording. I would agree, but I think we should limit the event to just be from when the console triggers it (so something like "RecordingFromConsole"). We already get this functionality for WebInspector UI triggered recordings via the callback/promise of CanvasAgent.startRecording, so it seems redundant to have it fire another event.
Matt Baker
Comment 16 2017-12-16 11:21:53 PST
(In reply to Devin Rousso from comment #15) > (In reply to Matt Baker from comment #14) > > Actually I think it will be a lot more straightforward to just have > > CanvasAgent dispatch RecordingStarted events, and include a `fromConsole` > > parameter, rather than having the UI synthesize events and infer the origin > > of the recording. > I would agree, but I think we should limit the event to just be from when > the console triggers it (so something like "RecordingFromConsole"). We > already get this functionality for WebInspector UI triggered recordings via > the callback/promise of CanvasAgent.startRecording, so it seems redundant to > have it fire another event. That makes sense. CanvasManager fires RecordingStarted when CanvasAgent.startRecording resolves, so we can add `fromConsole` to its event data, and use the same event when a RecordingStartedFromConsole is dispatches from the backend.
Joseph Pecoraro
Comment 17 2018-01-17 16:04:50 PST
When a quick recording like this finishes, we should immediately go directly into the Recording ContentView.
Matt Baker
Comment 18 2018-01-19 11:29:28 PST
(In reply to Matt Baker from comment #1) > Created attachment 328641 [details] > [Image] Multiple "Waiting for frames…" messages > > This patch also address an issue where ALL canvases would update their UI > when a recording begins. I split this off since it's a small fix: <https://webkit.org/b/181865> Web Inspector: Canvas Tab: Multiple "waiting for frames" messages displayed
Devin Rousso
Comment 19 2018-08-23 11:12:09 PDT
I think the changes made in <https://webkit.org/b/182995> and <https://webkit.org/b/185152> have resolved this bug.
Note You need to log in before you can comment on or make changes to this bug.