Web Inspector: Canvas: protocol error on first open

[Devin Rousso]

# STEPS TO REPRODUCE:
1. inspect any page
2. go to the Canvas tab
3. enable auto-capture
4. close Web Inspector
5. open Web Inspector
 => error in inspector2

[Error] Request with id = 2 failed. {"code":-32601,"message":"'Canvas' domain was not found","data":[{"code":-32601,"message":"'Canvas' domain was not found"}]}
	_dispatchResponse (Main.js:1246)
	dispatch (Main.js:1236)
	dispatch (Main.js:1154)
	dispatchNextQueuedMessageFromBackend (Main.js:1329)
[Error] Error: 'Canvas' domain was not found
	(anonymous function) (Main.js:24966:243)
	promiseReactionJob

[Radar WebKit Bug Importer]

<rdar://problem/48407871>

[Devin Rousso]

Created attachment 362996 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 362996 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:104
> +        try {
> +            await CanvasAgent.setRecordingAutoCaptureFrameCount(enabled ? count : 0);

I don't think this is the right approach.

1. initializeTarget should set this state on the target's CanvasAgent.

    initializeTarget(target)
    {
        if (target.CanvasAgent) {
            target.CanvasAgent.enable();
            target.CanvasAgent.setRecordingAutoCaptureFrameCount(<value>);
        }
    }

2. This should update the value and then update the value across all targets with CanvasAgent:

    for (let target of WI.targets) {
        if (target.CanvasAgent)
            target.CanvasAgent.setRecordingAutoCaptureFrameCount(<value>);
    }

That will make this code sufficiently safe for multi-targets.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:110
> +        } catch (e) {
> +            console.error(e);
> +        }

Would the try/catch be needed at all? Seems it would mask the nicer unhandledrejection display that helps us catch these!

[Devin Rousso]

Created attachment 363015 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 363015 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:46
> -        if (target.CanvasAgent)
> -            target.CanvasAgent.enable();
> +        if (!target.CanvasAgent)
> +            return;

The style in all of these initializeTargets is:

    if (target.FooAgent) {
        ...
    }

I'd prefer we keep that style consistent for now. Maybe we can convert most of these to early returns later.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:106
> +        if (!WI.targetsAvailable())
> +            await WI.whenTargetsAvailable();

This part should be unnecessary. When new targets become available the state will be set on them in initializeTarget, so we should not have to wait for them.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:113
> +                promises.push(target.CanvasAgent.setRecordingAutoCaptureFrameCount(enabled ? count : 0));
> +        }
> +        await Promise.all(promises);

Why do we await these at all? Even if a backend error happened, would we care?

I'd expect this to just be the simple loop and updating the settings:

    for (let target of WI.targets) {
        if (target.CanvasAgent)
            promises.push(target.CanvasAgent.setRecordingAutoCaptureFrameCount(enabled ? count : 0));
    }

    WI.settings.canvasRecordingAutoCaptureEnabled.value = enabled && count;
    WI.settings.canvasRecordingAutoCaptureFrameCount.value = count;

[Devin Rousso]

Comment on attachment 363015 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:106
>> +            await WI.whenTargetsAvailable();
> 
> This part should be unnecessary. When new targets become available the state will be set on them in initializeTarget, so we should not have to wait for them.

Good point!

>> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:113
>> +        await Promise.all(promises);
> 
> Why do we await these at all? Even if a backend error happened, would we care?
> 
> I'd expect this to just be the simple loop and updating the settings:
> 
>     for (let target of WI.targets) {
>         if (target.CanvasAgent)
>             promises.push(target.CanvasAgent.setRecordingAutoCaptureFrameCount(enabled ? count : 0));
>     }
> 
>     WI.settings.canvasRecordingAutoCaptureEnabled.value = enabled && count;
>     WI.settings.canvasRecordingAutoCaptureFrameCount.value = count;

I prefer avoid changing values in the frontend until we know that it actually "worked" in the backend, as this can help prevent data corruption (e.g. somehow passing a string as `count` instead of a number would "corrupt" the setting's value to be a string).

What I wrote doesn't do anything in the case of an error, so that would be thrown (and caught by the global error handler) as expected.

[Devin Rousso]

Created attachment 363426 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 363426 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:104
> +        if (WI.targetsAvailable()) {

Is this necessary? When no targets are available, WI.targets should be an empty list safe to iterate.

[Devin Rousso]

Comment on attachment 363426 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:104
>> +        if (WI.targetsAvailable()) {
> 
> Is this necessary? When no targets are available, WI.targets should be an empty list safe to iterate.

🤦‍♂️

[Devin Rousso]

Created attachment 363538 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 363538 [details]
Patch

Clearing flags on attachment: 363538

Committed r242374: <https://trac.webkit.org/changeset/242374>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.