Bug 177762 – Web Inspector: Improve CanvasManager recording events

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

[Matt Baker]

Summary:
Improve CanvasManager recording events.

1) Make CanvasManager interface consistent with TimelineManager:
  - Add RecordingStarted event
  - Rename RecordingFinished event to RecordingStopped

2) Remove hack where RecordingFinished was sent to signal that a recording failed to start.

[Matt Baker]

Created attachment 322406 [details]
Patch

[Devin Rousso]

Comment on attachment 322406 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322406&action=review

Other than the issue below, this looks fine to me.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:89
> +            this.dispatchEventToListeners(WI.CanvasManager.Event.RecordingStopped, {canvas, recording: null});

This has the possibility of firing `WI.CanvasManager.Event.RecordingStopped` twice: once from this point here, and then another time once the recording object is received in the frontend.  The tricky part, however, is that that won't always happen, as a recording with no actions will not call to `recordingFinished` (e.g. start and stop a recording without changing the canvas).  Maybe we should keep `RecordingFinished`, but also add `RecordingStopped`?

[Matt Baker]

Created attachment 322486 [details]
Patch

[Build Bot]

Comment on attachment 322486 [details]
Patch

Attachment 322486 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4743747

New failing tests:
inspector/canvas/recording.html
inspector/canvas/recording-2d.html
inspector/canvas/recording-webgl.html
inspector/canvas/recording-webgl-snapshots.html

[Build Bot]

Created attachment 322566 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 322486 [details]
Patch

Attachment 322486 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4744096

New failing tests:
inspector/canvas/recording.html
inspector/canvas/recording-2d.html
inspector/canvas/recording-webgl.html
inspector/canvas/recording-webgl-snapshots.html

[Build Bot]

Created attachment 322571 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Devin Rousso]

I think you forgot to upload the changes to LayoutTests, specifically `WI.CanvasManager.Event.RecordingFinished` -> `WI.CanvasManager.Event.RecordingStopped` :P

[Build Bot]

Comment on attachment 322486 [details]
Patch

Attachment 322486 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4744197

New failing tests:
inspector/canvas/recording.html
inspector/canvas/recording-2d.html
inspector/canvas/recording-webgl.html
inspector/canvas/recording-webgl-snapshots.html

[Build Bot]

Created attachment 322572 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Matt Baker]

Created attachment 322631 [details]
Patch

[Devin Rousso]

Comment on attachment 322631 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322631&action=review

r=me.  Good refactor =D

> Source/WebInspectorUI/ChangeLog:21
> +        Make requests of CanvasManager only, defer updating the UI.

Style: add newline after comments.

> Source/WebInspectorUI/ChangeLog:24
> +        Renamed to _recordingStarted.

I think you meant "recordingStopped".

> LayoutTests/inspector/canvas/recording-2d.html:453
> +                let frames = recording.toJSON().frames;

Why was the "toJSON" added?  The idea is to test the swizzling of each action's parameters and, in this case, ensure that NaN is converted into null.

[Matt Baker]

Created attachment 322695 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 322695 [details]
Patch for landing

Clearing flags on attachment: 322695

Committed r222888: <http://trac.webkit.org/changeset/222888>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34824278>