Summary: | Web Inspector: Canvas: improve recording controls and state management | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||||||
Component: | Web Inspector | Assignee: | 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
Matt Baker
2017-09-11 18:59:38 PDT
Created attachment 320511 [details]
Patch
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. |