[webkit-reviews] review denied: [Bug 172624] Web Inspector: Instrument shaders for WebGL canvas in the backend : [Attachment 316032] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 172624: Web Inspector: Instrument shaders for WebGL canvas in the backend
https://bugs.webkit.org/show_bug.cgi?id=172624

Attachment 316032: Patch

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




--- Comment #12 from Brian Burg <bburg at apple.com> ---
Comment on attachment 316032
  --> https://bugs.webkit.org/attachment.cgi?id=316032
Patch

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

I don't see any reason why this can't be split into smaller patches. You are
unlikely to get thorough code review given the current size. Splitting will
also make it more obvious where test coverage is lacking. For example, there is
no test of what happens when the canvas is deleted from the page, the user
tries to attach multiple programs, or what exactly happens at the protocol
level when the canvas context is lost and restored.

Here's a series you could use that would be easier to review, and let you test
things separately:

1. Program creation and deletion
2. Shader compilation
3. Attaching/detaching shaders
4. Linking programs
5. Any other error conditions (context lost/restored) if they need extra
handling.

Dean/Jeremy, are there any other error conditions / lifetime issues /
interactions that we are not thinking about right now?

> Source/WebCore/inspector/InspectorShaderProgram.h:40
> +class InspectorShaderProgram final : public
RefCounted<InspectorShaderProgram> {

Please put this in namespace Inspector and rename to simply ShaderProgram,
unless there's a reason not to (like template specializations from other
namespaces).

Do you think this class need to change/clone for WebGL2 / WebGPU?