Bug 185152

Created attachment 347468 [details] Patch

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>