Bug 177762 - Web Inspector: Improve CanvasManager recording events
Summary: Web Inspector: Improve CanvasManager recording events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks: WebInspectorCanvasRecording 177604
  Show dependency treegraph
 
Reported: 2017-10-02 11:55 PDT by Matt Baker
Modified: 2017-12-04 17:33 PST (History)
10 users (show)

See Also:


Attachments
Patch (7.59 KB, patch)
2017-10-02 12:03 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (12.71 KB, patch)
2017-10-02 19:25 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (2.52 MB, application/zip)
2017-10-03 12:52 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-elcapitan (1.11 MB, application/zip)
2017-10-03 13:11 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (3.05 MB, application/zip)
2017-10-03 13:30 PDT, Build Bot
no flags Details
Patch (23.25 KB, patch)
2017-10-03 22:11 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch for landing (22.95 KB, patch)
2017-10-04 11:36 PDT, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2017-10-02 11:55:41 PDT
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.
Comment 1 Matt Baker 2017-10-02 12:03:35 PDT
Created attachment 322406 [details]
Patch
Comment 2 Devin Rousso 2017-10-02 13:11:22 PDT
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`?
Comment 3 Matt Baker 2017-10-02 19:25:10 PDT
Created attachment 322486 [details]
Patch
Comment 4 Build Bot 2017-10-03 12:52:28 PDT
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
Comment 5 Build Bot 2017-10-03 12:52:30 PDT
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
Comment 6 Build Bot 2017-10-03 13:11:23 PDT
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
Comment 7 Build Bot 2017-10-03 13:11:24 PDT
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
Comment 8 Devin Rousso 2017-10-03 13:28:26 PDT
I think you forgot to upload the changes to LayoutTests, specifically `WI.CanvasManager.Event.RecordingFinished` -> `WI.CanvasManager.Event.RecordingStopped` :P
Comment 9 Build Bot 2017-10-03 13:30:41 PDT
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
Comment 10 Build Bot 2017-10-03 13:30:42 PDT
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
Comment 11 Matt Baker 2017-10-03 22:11:35 PDT
Created attachment 322631 [details]
Patch
Comment 12 Devin Rousso 2017-10-04 01:18:42 PDT
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.
Comment 13 Matt Baker 2017-10-04 11:36:19 PDT
Created attachment 322695 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2017-10-04 16:51:51 PDT
Comment on attachment 322695 [details]
Patch for landing

Clearing flags on attachment: 322695

Committed r222888: <http://trac.webkit.org/changeset/222888>
Comment 15 WebKit Commit Bot 2017-10-04 16:51:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2017-10-04 16:52:32 PDT
<rdar://problem/34824278>