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, drousso, 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

Description Matt Baker 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
Comment 1 Matt Baker 2017-09-11 19:04:14 PDT
Created attachment 320511 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2017-09-11 19:05:06 PDT
<rdar://problem/34382294>
Comment 3 Devin Rousso 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.
Comment 4 Matt Baker 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.
Comment 5 Matt Baker 2017-09-11 21:22:20 PDT
Created attachment 320515 [details]
Patch
Comment 6 Matt Baker 2017-09-11 21:24:46 PDT
Created attachment 320516 [details]
Patch for landing
Comment 7 WebKit Commit Bot 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>
Comment 8 WebKit Commit Bot 2017-09-11 22:06:11 PDT
All reviewed patches have been landed.  Closing bug.