Bug 176893

Summary: Web Inspector: REGRESSION(r221901): Single frame recordings don't reset the recording navigation item
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, inspector-bugzilla-changes, mattbaker, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 176762    
Bug Blocks: 173807    
Attachments:
Description Flags
Patch none

Description Devin Rousso 2017-09-14 01:03:30 PDT
* 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
Comment 1 Devin Rousso 2017-09-14 01:05:13 PDT
Created attachment 320744 [details]
Patch
Comment 2 Matt Baker 2017-09-14 09:50:09 PDT
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.
Comment 3 Devin Rousso 2017-09-18 10:23:29 PDT
(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.
Comment 4 Matt Baker 2017-09-18 11:26:39 PDT
(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.
Comment 5 Matt Baker 2017-09-18 11:27:11 PDT
Comment on attachment 320744 [details]
Patch

r=me
Comment 6 WebKit Commit Bot 2017-09-18 11:56:42 PDT
Comment on attachment 320744 [details]
Patch

Clearing flags on attachment: 320744

Committed r222171: <http://trac.webkit.org/changeset/222171>
Comment 7 WebKit Commit Bot 2017-09-18 11:56:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2017-09-27 12:30:10 PDT
<rdar://problem/34693389>