Web Inspector: REGRESSION(r221901): Single frame recordings don't reset the recording navigation item

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

* Steps to reproduce:
1. Open WebInspector on a page with a 2D <canvas>
2. Select the 2D <canvas> in the Resources tab
3. Shift-click the recording navigation item to record a single frame
4. Wait for the recording tab to appear once the recording has finished
5. Switch back to the Resources tab
 => The recording navigation item still shows the grey "stop" box, even though the recording has finished

[Devin Rousso]

Created attachment 320744 [details]
Patch

[Matt Baker]

Comment on attachment 320744 [details]
Patch

r-, because we should be able to add test coverage for this. In recording2d.html whenever RecordingFinished is received, we should check that WI.CanvasManager.recordingCanvas is null.

I should have added this test when introducing this change.

[Devin Rousso]

(In reply to Matt Baker from comment #2)
> Comment on attachment 320744 [details]
> Patch
> 
> r-, because we should be able to add test coverage for this. In
> recording2d.html whenever RecordingFinished is received, we should check
> that WI.CanvasManager.recordingCanvas is null.
> 
> I should have added this test when introducing this change.

None of the tests use `CanvasManager.prototype.startRecording`.  All the recording tests directly call `requestRecording`/`cancelRecording` on `CanvasAgent`.  The issue is that the `CanvasManager` functions don't expose the `error` object or allow for a callback, both of which are needed in the tests.

[Matt Baker]

(In reply to Devin Rousso from comment #3)
> (In reply to Matt Baker from comment #2)
> > Comment on attachment 320744 [details]
> > Patch
> > 
> > r-, because we should be able to add test coverage for this. In
> > recording2d.html whenever RecordingFinished is received, we should check
> > that WI.CanvasManager.recordingCanvas is null.
> > 
> > I should have added this test when introducing this change.
> 
> None of the tests use `CanvasManager.prototype.startRecording`.  All the
> recording tests directly call `requestRecording`/`cancelRecording` on
> `CanvasAgent`.  The issue is that the `CanvasManager` functions don't expose
> the `error` object or allow for a callback, both of which are needed in the
> tests.

If CanvasManager isn't testable, then we should take steps to address that. Updating it to be promise-based would be one solution. We could also rely more of event dispatched from the manager. Currently, when requestRecording fails CanvasManager dispatches a RecordingFinished event with a null recording. This is enough information to write a test, as long as you don't care about the actual error message.

[Matt Baker]

Comment on attachment 320744 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 320744 [details]
Patch

Clearing flags on attachment: 320744

Committed r222171: <http://trac.webkit.org/changeset/222171>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693389>