Summary: | Web Inspector: Canvas Tab: Improve feedback after stopping a recording | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||||||||||||
Component: | Web Inspector | Assignee: | 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
Matt Baker
2017-12-06 15:53:12 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.
Created attachment 328646 [details]
Patch
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 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
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 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
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 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
Created attachment 328684 [details]
Patch
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. 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 (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. 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. (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. (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. When a quick recording like this finishes, we should immediately go directly into the Recording ContentView. (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 I think the changes made in <https://webkit.org/b/182995> and <https://webkit.org/b/185152> have resolved this bug. |