[webkit-reviews] review granted: [Bug 176762] Web Inspector: Canvas: improve recording controls and state management : [Attachment 320511] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <webkit at devinrousso.com> has granted Matt Baker
<mattbaker at apple.com>'s request for review:
Bug 176762: Web Inspector: Canvas: improve recording controls and state
management
https://bugs.webkit.org/show_bug.cgi?id=176762

Attachment 320511: Patch

https://bugs.webkit.org/attachment.cgi?id=320511&action=review




--- Comment #3 from Devin Rousso <webkit at devinrousso.com> ---
Comment on attachment 320511
  --> https://bugs.webkit.org/attachment.cgi?id=320511
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.