Bug 176893

Summary:

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

Product:

WebKit

Reporter:

Devin Rousso <hi>

Component:

Web Inspector

Assignee:

Devin Rousso <hi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, inspector-bugzilla-changes, mattbaker, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Bug Depends on:

176762

Bug Blocks:

173807

Attachments:

Description	Flags
Patch	none

Devin Rousso

Reported 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

Attachments
Patch (1.80 KB, patch) 2017-09-14 01:05 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Devin Rousso

Comment 1 2017-09-14 01:05:13 PDT

Created attachment 320744 [details] Patch

Matt Baker

Comment 2 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.

Devin Rousso

Comment 3 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.

Matt Baker

Comment 4 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.

Matt Baker

Comment 5 2017-09-18 11:27:11 PDT

Comment on attachment 320744 [details] Patch r=me

WebKit Commit Bot

Comment 6 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>

WebKit Commit Bot

Comment 7 2017-09-18 11:56:44 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 8 2017-09-27 12:30:10 PDT

<rdar://problem/34693389>

Note You need to log in before you can comment on or make changes to this bug.