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
Created attachment 320511 [details] Patch
<rdar://problem/34382294>
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.
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.
Created attachment 320515 [details] Patch
Created attachment 320516 [details] Patch for landing
Comment on attachment 320516 [details] Patch for landing Clearing flags on attachment: 320516 Committed r221901: <http://trac.webkit.org/changeset/221901>
All reviewed patches have been landed. Closing bug.