[webkit-reviews] review denied: [Bug 87960] Web Inspector: [WebGL] Add WebGL instrumentation support on the backend : [Attachment 145251] Patch

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has denied Andrey Adaikin
<aandrey at chromium.org>'s request for review:
Bug 87960: Web Inspector: [WebGL] Add WebGL instrumentation support on the
backend
https://bugs.webkit.org/show_bug.cgi?id=87960

Attachment 145251: Patch
https://bugs.webkit.org/attachment.cgi?id=145251&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=145251&action=review


> Source/WebCore/CMakeLists.txt:2602
> +ADD_CUSTOM_COMMAND(

Should we check if WebGL is enabled here?

> Source/WebCore/bindings/js/JSInjectedScriptManager.cpp:87
> +    // FIXME(87975): Implement this!

It shouldn't be hard to implement this for JSC, we already do similar things
with the inspector injected script.

> Source/WebCore/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp:94
> +	   if (!wrapped.v8Value().IsEmpty())

This code will create ScriptState instance for all contexts even if WebGL
instrumentation is disabled. This should be fixed. We should probably check if
WebGL inspection is enabled before getting ScriptState.

> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:-60
> -    v8::Local<v8::Function> function =
V8InjectedScriptHost::GetTemplate()->GetFunction();

These changes are not needed anymore, please revert them if so.

> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:129
> +    v8::Local<v8::Object> glContextObject =
toV8InLocalContext(V8WebGLRenderingContext::GetTemplate(),
&V8WebGLRenderingContext::info, glContext);

You should be able to use standard toV8 mechanism here, it will create a
binding for you which you will wrap with the instrumenting code. Existing code
could probably changed as well I need to check.