Bug 180509 – Web Inspector: Canvas Tab: Improve feedback after stopping a recording

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

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.

[Matt Baker]

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]

Created attachment 328646 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/35895738>

[EWS Watchlist]

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]

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

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

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]

Created attachment 328684 [details]
Patch

[Blaze Burg]

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

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

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]

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

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

When a quick recording like this finishes, we should immediately go directly into the Recording ContentView.

[Matt Baker]

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

I think the changes made in <https://webkit.org/b/182995> and <https://webkit.org/b/185152> have resolved this bug.