Bug 182995 – Web Inspector: Canvas tab: determine hasVisibleEffect for all actions immediately after recording is added

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

[Devin Rousso]

Instead of trying to spread the work of determining `hasVisibleEffect`, which has to compare a before/after of the canvas' content, to until the action has been applied for the first time, since we now give status indications for every frame captured, we can use the same idea to do all of this work at the beginning too.

[Devin Rousso]

Created attachment 334342 [details]
Patch

[Devin Rousso]

Created attachment 334343 [details]
[Image] After Patch is applied

[Devin Rousso]

Created attachment 334420 [details]
Patch

[Devin Rousso]

Created attachment 335763 [details]
Patch

Switch from RAF to YieldableTask

[Matt Baker]

Comment on attachment 335763 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=335763&action=review

Really nice work! I think this is almost ready to land. Some general feedback:

1) The UI works as I'd expect when importing a recording too. Nice!
2) I noticed it's possible to start a recording while the current one is still processing. It worked! However the UI stays in the Processing view, instead of switching to the new recording's view.
3) Processing a large recording can take a long time -- on the order of minutes. The ability to cancel processing would be ideal.
  - What should cancelling do? Should unprocessed frames be truncated, allowing the user to benefit from the work already done? Or should we throw out the whole thing? If we do the former, we'd want to indicate to the user that it was truncated.
  - The ability to cancel can be addressed in a follow-up. This patch is a huge improvement by itself.
4) Processing a single frame (or very short recording) flashes the progress UI briefly. It would be nice to not show it when it's only going to be around for a second or so.
  - This could be a follow-up if necessary.

> Source/WebInspectorUI/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

This is a significant change to the recording UI. I think it deserves a brief (just a couple of sentences) summary, mentioning the switch to YieldableTask and addition of progress bar UI.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:162
> +    process() {

Style: open brace needs to be on its own line.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:166
> +            Promise.all(this._actions.map((action) => action.swizzle(this))).then(async () => {

Neat! I like the async anonymous function.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:167
> +                let context = this.createContext();

Nit: I'd move this below line 169, since it isn't used until after the await returns.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:237
> +                let task = new WI.YieldableTask(this, createIteratorForActionsToBeApplied.call(this), {idleInterval: 1000 / 60});

Why are we idling for the length of a 60 fps frame?

I removed the idle as a test, and noticed no decline in UI responsiveness (as measured by resizing the recording sidebar during processing). It also improved the load time of a 211KB recording from 23s to 12s. Is there a compelling reason to keep an idle time?

> Source/WebInspectorUI/UserInterface/Models/Recording.js:409
> +        this.dispatchEventToListeners(WI.Recording.Event.ProcessedAction, item);

I know this works, but by convention event data should be supplied as {item}.

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:533
> +        progressElement.value = event.data.index / this.representedObject.actions.length;

Should be `event.data.item.index` (see earlier comment).

[Devin Rousso]

Comment on attachment 335763 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=335763&action=review

>> Source/WebInspectorUI/UserInterface/Models/Recording.js:167
>> +                let context = this.createContext();
> 
> Nit: I'd move this below line 169, since it isn't used until after the await returns.

I would prefer to have this before.  In my mind, it flows nicer to have `initialContent` be initialized and then immediately used, instead of initializing it, creating the canvas, and then using it.  If you would strongly prefer it the other way, I can change it though.

>> Source/WebInspectorUI/UserInterface/Models/Recording.js:237
>> +                let task = new WI.YieldableTask(this, createIteratorForActionsToBeApplied.call(this), {idleInterval: 1000 / 60});
> 
> Why are we idling for the length of a 60 fps frame?
> 
> I removed the idle as a test, and noticed no decline in UI responsiveness (as measured by resizing the recording sidebar during processing). It also improved the load time of a 211KB recording from 23s to 12s. Is there a compelling reason to keep an idle time?

I was going off the only existing use of `WI.YieldableTask` (which is in `WI.DataGrid`), but I can't really remember why I chose that number 😅 My guess is that I was thinking that since `WI.YieldableTask` uses `setTimeout`, if we had an `idleInterval` of 0, it'd keep scheduling for the next tick and might prevent other frames from going, but that doesn't really make much sense even as I'm writing this.

Are you suggesting we remove it entirely, or set it to a much smaller number?

[Matt Baker]

Comment on attachment 335763 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=335763&action=review

>>> Source/WebInspectorUI/UserInterface/Models/Recording.js:237
>>> +                let task = new WI.YieldableTask(this, createIteratorForActionsToBeApplied.call(this), {idleInterval: 1000 / 60});
>> 
>> Why are we idling for the length of a 60 fps frame?
>> 
>> I removed the idle as a test, and noticed no decline in UI responsiveness (as measured by resizing the recording sidebar during processing). It also improved the load time of a 211KB recording from 23s to 12s. Is there a compelling reason to keep an idle time?
> 
> I was going off the only existing use of `WI.YieldableTask` (which is in `WI.DataGrid`), but I can't really remember why I chose that number 😅 My guess is that I was thinking that since `WI.YieldableTask` uses `setTimeout`, if we had an `idleInterval` of 0, it'd keep scheduling for the next tick and might prevent other frames from going, but that doesn't really make much sense even as I'm writing this.
> 
> Are you suggesting we remove it entirely, or set it to a much smaller number?

Let's remove it. In my testing removing it didn't have an appreciable impact on UI responsiveness, but did speed up the recording processing.

[Devin Rousso]

(In reply to Matt Baker from comment #5)
> 2) I noticed it's possible to start a recording while the current one is
> still processing. It worked! However the UI stays in the Processing view,
> instead of switching to the new recording's view.
Hmm.  I'll see if I can figure out why it's doing that.

> 3) Processing a large recording can take a long time -- on the order of
> minutes. The ability to cancel processing would be ideal.
>   - What should cancelling do? Should unprocessed frames be truncated,
> allowing the user to benefit from the work already done? Or should we throw
> out the whole thing? If we do the former, we'd want to indicate to the user
> that it was truncated.
I think the former makes more sense.  For the remaining actions, I think we should add a "Process Remaining" button to the end of the actions list in the navigation sidebar that, when clicked, restarts the processing process from where it left off.

>   - The ability to cancel can be addressed in a follow-up. This patch is a
> huge improvement by itself.
<https://webkit.org/b/185152>

> 4) Processing a single frame (or very short recording) flashes the progress
> UI briefly. It would be nice to not show it when it's only going to be
> around for a second or so.
I'll rework `yieldableTaskWillProcessItem` so that it only creates the message view after the first `yieldableTaskDidYield`.

>   - This could be a follow-up if necessary.
<https://webkit.org/b/185153>

[Matt Baker]

Comment on attachment 335763 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=335763&action=review

>>> Source/WebInspectorUI/UserInterface/Models/Recording.js:167
>>> +                let context = this.createContext();
>> 
>> Nit: I'd move this below line 169, since it isn't used until after the await returns.
> 
> I would prefer to have this before.  In my mind, it flows nicer to have `initialContent` be initialized and then immediately used, instead of initializing it, creating the canvas, and then using it.  If you would strongly prefer it the other way, I can change it though.

Not a big deal, just a nit.

[Devin Rousso]

(In reply to Devin Rousso from comment #8)
> (In reply to Matt Baker from comment #5)
> > 2) I noticed it's possible to start a recording while the current one is
> > still processing. It worked! However the UI stays in the Processing view,
> > instead of switching to the new recording's view.
> Hmm.  I'll see if I can figure out why it's doing that.
This was an issue that existed before this patch.  Thinking about it, I'm not sure if we want to automatically switch to the canvas view when a recording is started.  Considering how we don't forcibly show the `WI.CanvasContentView` when a user starts a recording from the overview, I don't think we want to forcibly switch away from a selected recording if the user starts a new one.

As an aside, this might be slightly alleviated by <https://webkit.org/b/182950>.

[Devin Rousso]

Created attachment 339191 [details]
Patch

Fixed an issue with WebGL canvases.  Also turned the `swizzle` phase into its own `WI.YieldableTask` for better performance.

[Devin Rousso]

Created attachment 339193 [details]
Patch

Clean up ChangeLog

[Matt Baker]

Comment on attachment 339193 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339193&action=review

r=me, very nice! Just a couple minor comments.

Also, I'm not sure about having two progress bars, but I'm okay with it for now. We can revisit later if needed.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:170
> +            function* createIteratorForActions() {

A generator isn't necessary, you can just use an array:

let items = this._actions.map((action, index) => { return {action, index} });
this._swizzleTask = new WI.YieldableTask(this, items);
this._applyTask = new WI.YieldableTask(this, items);

> Source/WebInspectorUI/UserInterface/Models/Recording.js:362
> +        this.dispatchEventToListeners(WI.Recording.Event.ProcessedActionApply, {index: item.index});

Style: indent

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:283
> +        let wasValid = this._valid;

The temporary can be eliminated:

markInvalid()
{
    if (!this._valid)
        return;

    this._valid = false;
    this.dispatchEventToListeners(...)
}

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:457
> +            if (!this._processing)

Combine these checks:

if (this._showPathButtonNavigationItem.activated !== activated && !this._processing)
    this._generateContentCanvas2D(this._index);

[Devin Rousso]

Created attachment 339228 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 339228 [details]
Patch

Clearing flags on attachment: 339228

Committed r231218: <https://trac.webkit.org/changeset/231218>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39884139>