Bug 177762

Summary:

Web Inspector: Improve CanvasManager recording events

Product:

WebKit

Reporter:

Matt Baker <mattbaker>

Component:

Web Inspector

Assignee:

Matt Baker <mattbaker>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, hi, inspector-bugzilla-changes, keith_miller, mark.lam, msaboff, rniwa, saam, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

173807, 177604

Attachments:

Description	Flags
Patch	none
Patch	none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2	none
Archive of layout-test-results from ews102 for mac-elcapitan	none
Archive of layout-test-results from ews117 for mac-elcapitan	none
Patch	none
Patch for landing	none

Matt Baker

Reported 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.

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Matt Baker

Comment 1 2017-10-02 12:03:35 PDT

Created attachment 322406 [details] Patch

Devin Rousso

Comment 2 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`?

Matt Baker

Comment 3 2017-10-02 19:25:10 PDT

Created attachment 322486 [details] Patch

Build Bot

Comment 4 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

Build Bot

Comment 5 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

Build Bot

Comment 6 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

Build Bot

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

Devin Rousso

Comment 8 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

Build Bot

Comment 9 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

Build Bot

Comment 10 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

Matt Baker

Comment 11 2017-10-03 22:11:35 PDT

Created attachment 322631 [details] Patch

Devin Rousso

Comment 12 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.

Matt Baker

Comment 13 2017-10-04 11:36:19 PDT

Created attachment 322695 [details] Patch for landing

WebKit Commit Bot

Comment 14 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>

WebKit Commit Bot

Comment 15 2017-10-04 16:51:53 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 16 2017-10-04 16:52:32 PDT

<rdar://problem/34824278>

Note You need to log in before you can comment on or make changes to this bug.