Bug 185152

Summary: Web Inspector: Canvas tab: allow recording processing to be stopped midway
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=185153
https://bugs.webkit.org/show_bug.cgi?id=180596
https://bugs.webkit.org/show_bug.cgi?id=180509
https://bugs.webkit.org/show_bug.cgi?id=180838
Bug Depends on: 182995    
Bug Blocks: 173807, 175485, 188808    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews206 for win-future
none
Patch
none
Patch none

Description Devin Rousso 2018-04-30 18:26:58 PDT
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.
Comment 1 Devin Rousso 2018-08-19 15:00:00 PDT
Created attachment 347468 [details]
Patch
Comment 2 EWS Watchlist 2018-08-19 17:11:05 PDT Comment hidden (obsolete)
Comment 3 EWS Watchlist 2018-08-19 17:11:17 PDT Comment hidden (obsolete)
Comment 4 Joseph Pecoraro 2018-08-20 12:03:34 PDT
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 5 Devin Rousso 2018-08-20 13:38:43 PDT
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.
Comment 6 Devin Rousso 2018-08-20 13:39:24 PDT
Created attachment 347536 [details]
Patch
Comment 7 Devin Rousso 2018-08-20 13:48:20 PDT
Created attachment 347538 [details]
Patch

Rebase
Comment 8 WebKit Commit Bot 2018-08-20 14:32:03 PDT
Comment on attachment 347538 [details]
Patch

Clearing flags on attachment: 347538

Committed r235095: <https://trac.webkit.org/changeset/235095>
Comment 9 WebKit Commit Bot 2018-08-20 14:32:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2018-08-20 16:49:48 PDT
<rdar://problem/43534371>