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

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied 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 159366: Patch
https://bugs.webkit.org/attachment.cgi?id=159366&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=159366&action=review


> Source/WebCore/inspector/InjectedScriptSource.js:576
> +	       inspectedWindow.console.error("Web Inspector error: A function
was expected for module %s evaluation", name);

We should not log into console. And this should never happen, right?

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:39
> +var Cache = function()

function Cache()
{
...

Please annotate and add compilation phase into the compile-front-end.py

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:45
> +    get size()

Avoid getters for better compilability.
size: function()
{
...
}

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:71
> +    }

This class looks like a Map from utilities.js a lot. Also, why not to use
Object.create(null) and Object.keys().length for the size computation?

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:75
> + * @param {Array|Arguments} args

Please use alphabetical order for @ annotations. (@constructor goes first here)


> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:78
> +var Call = function(resource, functionName, args, result)

function Call ...

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:122
> +    if (!obj || obj instanceof Resource)

When does this happen? (the instanceof Resource case)

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:143
> +	   this._wrappedObject = value;

Can we pass this in the constructor?

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:160
> +	   this._resourceManager = value;

When is this called?

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:170
> + * @extends {Resource}

swap lines

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:193
> +	   function processProperty(property) {

{ should go on the next line.

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:222
> +	   return function() {

{ on the next line

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:224
> +	       if (!manager || !manager.capturing)

When does it have no manager?