Bug 176762

Summary: Web Inspector: Canvas: improve recording controls and state management
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hi, inspector-bugzilla-changes, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 173807, 176893    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Matt Baker
Reported 2017-09-11 18:59:38 PDT
Summary: Improve recording controls and state management. Issue #1: The Canvas model toggles the recording, and call methods on CanvasAgent. - Fixing the layering issue by moving this into CanvasManager will make it simple to address issue #2. Issue #2: The record button can become out of sync with the current recording state: - Update to reflect the current state when CanvasContentView is shown - Disable when the CanvasContentView is not the active recording Steps to Reproduce: 1. Goto devinrousso.com 2. Start a canvas recording 3. Close the Resources tab 4. Reopen tab and canvas view => Recording is still in progress, record button is red (not toggled) 1. Goto https://developer.mozilla.org/en-US/docs/Web/API/Canvas_API/Tutorial/Basic_animations 2. Start a canvas recording 3. Switch to another canvas view => Record button is enabled, even though another canvas is recording
Attachments
Patch (7.13 KB, patch)
2017-09-11 19:04 PDT, Matt Baker 		no flags
DetailsFormatted DiffDiff
Patch (7.68 KB, patch)
2017-09-11 21:22 PDT, Matt Baker 		no flags
DetailsFormatted DiffDiff
Patch for landing (7.67 KB, patch)
2017-09-11 21:24 PDT, Matt Baker 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Matt Baker
Comment 1 2017-09-11 19:04:14 PDT
Created attachment 320511 [details] Patch
Radar WebKit Bug Importer
Comment 2 2017-09-11 19:05:06 PDT
<rdar://problem/34382294>
Devin Rousso
Comment 3 2017-09-11 21:04:11 PDT
Comment on attachment 320511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320511&action=review r=me, with a few questions/clarifications > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:58 > + console.assert(!this._recordingCanvas, "Recording already started."); Are we only going to allow one recording to be captured at a time? Totally fine either way. I just want to clarify. > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:71 > + this.dispatchEventToListeners(WI.CanvasManager.RecordingFinished, {canvas, recording: null}); Typo: WI.CanvasManager.Event.RecordingFinished Does this cause any issues when calling `WI.showRepresentedObject(event.data.recording);` since `recording` is `null`? > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.css:44 > +.navigation-bar > .item.canvas-record.disabled { Does the `.disabled` state not already apply styling, or is it just not obvious enough? > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:103 > + if (this._recordButtonNavigationItem.toggled) Instead of checking the state of the NavigationItem, I think it's cleaner to check to see if `WI.canvasManager.recordingCanvas` is truthy.
Matt Baker
Comment 4 2017-09-11 21:11:58 PDT
Comment on attachment 320511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320511&action=review >> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:58 >> + console.assert(!this._recordingCanvas, "Recording already started."); > > Are we only going to allow one recording to be captured at a time? Totally fine either way. I just want to clarify. We don't have plans to support multiple recordings anytime soon, but that isn't to say it will never happen. I made these changes with scaling in mind. >> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:71 >> + this.dispatchEventToListeners(WI.CanvasManager.RecordingFinished, {canvas, recording: null}); > > Typo: WI.CanvasManager.Event.RecordingFinished > > Does this cause any issues when calling `WI.showRepresentedObject(event.data.recording);` since `recording` is `null`? Will add a check in the event handler. >> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.css:44 >> +.navigation-bar > .item.canvas-record.disabled { > > Does the `.disabled` state not already apply styling, or is it just not obvious enough? Surprisingly, there was no styling. > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:120 > WI.showRepresentedObject(event.data.recording); Will add a check for a null recording here, since RecordingFinished now notifies for recordings that were requested but failed to start in the backend. I don't think we need to do anything fancy in the null case, just log the error.
Matt Baker
Comment 5 2017-09-11 21:22:20 PDT
Created attachment 320515 [details] Patch
Matt Baker
Comment 6 2017-09-11 21:24:46 PDT
Created attachment 320516 [details] Patch for landing
WebKit Commit Bot
Comment 7 2017-09-11 22:06:10 PDT
Comment on attachment 320516 [details] Patch for landing Clearing flags on attachment: 320516 Committed r221901: <http://trac.webkit.org/changeset/221901>
WebKit Commit Bot
Comment 8 2017-09-11 22:06:11 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.