[webkit-reviews] review canceled: [Bug 90597] Web Inspector: [WebGL] Generic framework draft for tracking WebGL resources : [Attachment 154066] Patch

[bugzilla-daemon at webkit.org]

Andrey Kosyakov <caseq at chromium.org> has canceled Andrey Adaikin
<aandrey at chromium.org>'s request for review:
Bug 90597: Web Inspector: [WebGL] Generic framework draft for tracking WebGL
resources
https://bugs.webkit.org/show_bug.cgi?id=90597

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

------- Additional Comments from Andrey Kosyakov <caseq at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=154066&action=review


> Source/WebCore/ChangeLog:3
> +	   Web Inspector: [WebGL] Generic framework draft for tracking WebGL
resources

Could you please describe the change in greater detail?

> Source/WebCore/inspector/InjectedScriptSource.js:420
>      injectModule: function(name, source)

Here and below, could you please annotate this?

> Source/WebCore/inspector/InjectedScriptSource.js:425
> +	   if (typeof moduleFunction !== "function")
> +	       return "A function was expected.";

Either throw or log error and return undefined?

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:74
> +	   this._args = Array.prototype.slice.call(this._args, 0);

Any reasons not to do this in the constructor?

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:117
> +    this._calls = [];

How is this used?

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:155
> +	   if (!this._resourceManager)
> +	       this._resourceManager = new ResourceTrackingManager();

So a resource may implicitly get a manager different from others?

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:284
> +		   var customWrapFunction =
WebGLRenderingContextResource.WrapFunction.prototype[property];

So we use fields of WrapFunction.prototype for custom wrapping of object
functions, yet it appears to have properties like 'call' or 'result' -- don't
they conflict?

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:306
> +	       processProperty(property);

glContext.keys.forEach(processProperty)?

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:423
> +    replay: function()

Could you please drop the methods that are unused and/or have no effect? This
would make the patch easier to review.

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:507
> +	       var name = object.constructor + "";

use toString()?

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:519
> +	   name = object + "";

ditto