[webkit-reviews] review denied: [Bug 195059] Web Inspector: Canvas: protocol error on first open : [Attachment 363015] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 195059: Web Inspector: Canvas: protocol error on first open
https://bugs.webkit.org/show_bug.cgi?id=195059

Attachment 363015: Patch

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




--- Comment #5 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 363015
  --> https://bugs.webkit.org/attachment.cgi?id=363015
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;