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: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 173807, 177604 | ||||||||||||||||||
Attachments: |
|
Description
Matt Baker
2017-10-02 11:55:41 PDT
Created attachment 322406 [details]
Patch
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`? Created attachment 322486 [details]
Patch
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 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 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 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
I think you forgot to upload the changes to LayoutTests, specifically `WI.CanvasManager.Event.RecordingFinished` -> `WI.CanvasManager.Event.RecordingStopped` :P 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 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
Created attachment 322631 [details]
Patch
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. Created attachment 322695 [details]
Patch for landing
Comment on attachment 322695 [details] Patch for landing Clearing flags on attachment: 322695 Committed r222888: <http://trac.webkit.org/changeset/222888> All reviewed patches have been landed. Closing bug. |