If the user accidentally records more frames than needed, or just wants to view part of the recording, we should let them "stop" the processing the recording and view what is already there. Additionally, we should create a button to resume processing if they want to view the the rest of the recording.
Created attachment 347468 [details] Patch
Comment on attachment 347468 [details] Patch Attachment 347468 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8911986 New failing tests: http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html
Created attachment 347471 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 347468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347468&action=review r=me > Source/WebInspectorUI/UserInterface/Models/Recording.js:173 > + console.assert(!this.ready, "Cannot stop a completed process()."); Should this message be: "Cannot start an already started process()." > Source/WebInspectorUI/UserInterface/Models/Recording.js:414 > + if (!this._actions[0].ready) { > + this._actions[0].process(this, this._processContext); > + this.dispatchEventToListeners(WI.Recording.Event.ProcessedAction, {action: this._actions[0], index: 0}); > + } This deserves a comment, since its a special action without a frame. > Source/WebInspectorUI/UserInterface/Models/Recording.js:428 > + let cumulativeActionIndex = 0; > + for (let frameIndex = 0; frameIndex < this._frames.length; ++frameIndex) { > + let frame = this._frames[frameIndex]; > + > + for (let actionIndex = 0; actionIndex < frame.actions.length; ++actionIndex) { > + ++cumulativeActionIndex; > + > + let action = frame.actions[actionIndex]; > + if (action.ready) > + continue; The one sorta bad thing about this for pause + resume is that when you resume it loops through all of the previously processed frames and actions. You can short-circuit that, and skip frames that have been processed already: (I'm assuming a Frame's actions is never an empty list) let frame = this._frames[frameIndex]; if (frame.actions.lastValue.ready) continue; This might save some time resuming a super long recording of a canvas with many actions (I'm imagining 3D canvases fall into this bucket). > Source/WebInspectorUI/UserInterface/Models/Recording.js:438 > + if (!actionIndex) > + this.dispatchEventToListeners(WI.Recording.Event.ProcessedFrame, {frame, index: frameIndex}); This seems backwards to me. The name indicates to me that the frame has been processed, but here it seems to happen at index 0, so it seems to be when the frame has started to be processed. Yes the ProcessedAction event happens after the action has been processed. So these names appear to be inconsistent. > Source/WebInspectorUI/UserInterface/Models/Recording.js:443 > + await Promise.delay(); This is where the magic happens but its not entirely obvious. It may deserve a comment. // yield > Source/WebInspectorUI/UserInterface/Views/GeneralTreeElement.js:55 > + return this._statusElement; Note that the other paths typically create the sub-elements if needed. Is that needed here too, or is it expected that this may return undefined?
Comment on attachment 347468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347468&action=review >> Source/WebInspectorUI/UserInterface/Models/Recording.js:438 >> + this.dispatchEventToListeners(WI.Recording.Event.ProcessedFrame, {frame, index: frameIndex}); > > This seems backwards to me. The name indicates to me that the frame has been processed, but here it seems to happen at index 0, so it seems to be when the frame has started to be processed. Yes the ProcessedAction event happens after the action has been processed. So these names appear to be inconsistent. I did it this way because I want this event to fire before `WI.Recording.Event.ProcessedAction` so that `WI.CanvasSidebarPanel` has a chance to create the `WI.FolderTreeElement` before any actions are added to it. I'll update the name. >> Source/WebInspectorUI/UserInterface/Views/GeneralTreeElement.js:55 >> + return this._statusElement; > > Note that the other paths typically create the sub-elements if needed. Is that needed here too, or is it expected that this may return undefined? The `_statusElement` is only created if a `status` is provided, so it is possible for it to be undefined.
Created attachment 347536 [details] Patch
Created attachment 347538 [details] Patch Rebase
Comment on attachment 347538 [details] Patch Clearing flags on attachment: 347538 Committed r235095: <https://trac.webkit.org/changeset/235095>
All reviewed patches have been landed. Closing bug.
<rdar://problem/43534371>