Bug 176762 – Web Inspector: Canvas: improve recording controls and state management

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

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

[Matt Baker]

Created attachment 320511 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/34382294>

[Devin Rousso]

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 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]

Created attachment 320515 [details]
Patch

[Matt Baker]

Created attachment 320516 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 320516 [details]
Patch for landing

Clearing flags on attachment: 320516

Committed r221901: <http://trac.webkit.org/changeset/221901>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.