Bug 185152 – Web Inspector: Canvas tab: allow recording processing to be stopped midway

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 347468 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Joseph Pecoraro]

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?

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 347536 [details]
Patch

[Devin Rousso]

Created attachment 347538 [details]
Patch

Rebase

[WebKit Commit Bot]

Comment on attachment 347538 [details]
Patch

Clearing flags on attachment: 347538

Committed r235095: <https://trac.webkit.org/changeset/235095>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43534371>