Bug 180509 - Web Inspector: Canvas Tab: Improve feedback after stopping a recording
Summary: Web Inspector: Canvas Tab: Improve feedback after stopping a recording
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P1 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on: 178744
Blocks: WebInspectorCanvasRecording WebInspectorCanvasTab
  Show dependency treegraph
 
Reported: 2017-12-06 15:53 PST by Matt Baker
Modified: 2018-08-23 11:15 PDT (History)
7 users (show)

See Also:


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 Details | Formatted Diff | Diff
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-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 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.
Comment 1 Matt Baker 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.
Comment 2 Matt Baker 2017-12-06 16:17:47 PST
Created attachment 328646 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2017-12-06 16:18:11 PST
<rdar://problem/35895738>
Comment 4 EWS Watchlist 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
Comment 5 EWS Watchlist 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
Comment 6 EWS Watchlist 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
Comment 7 EWS Watchlist 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
Comment 8 EWS Watchlist 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
Comment 9 EWS Watchlist 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
Comment 10 Matt Baker 2017-12-06 23:37:31 PST
Created attachment 328684 [details]
Patch
Comment 11 Brian Burg 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.
Comment 12 Devin Rousso 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
Comment 13 Matt Baker 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.
Comment 14 Matt Baker 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.
Comment 15 Devin Rousso 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.
Comment 16 Matt Baker 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.
Comment 17 Joseph Pecoraro 2018-01-17 16:04:50 PST
When a quick recording like this finishes, we should immediately go directly into the Recording ContentView.
Comment 18 Matt Baker 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
Comment 19 Devin Rousso 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.