Bug 90597 – Web Inspector: [WebGL] Generic framework draft for tracking WebGL resources

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Andrey Adaikin]

The goal is to create a fairly generic framework for tracking a certain set of JS objects and their states (we will be interested in WebGL and 2D canvas). The framework should not dependent much on the Web Inspector code.

[Andrey Adaikin]

Created attachment 150911 [details]
Work in progress

[Andrey Adaikin]

Created attachment 154066 [details]
Patch

[Andrey Adaikin]

An intermediate step with a lot of FIXME's to be addressed later.
What is working already: wrapping GL context and textures, capturing function calls.

[Andrey Kosyakov]

Comment on attachment 154066 [details]
Patch

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

[Andrey Adaikin]

Comment on attachment 154066 [details]
Patch

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

>> Source/WebCore/inspector/InjectedScriptSource.js:420
>>      injectModule: function(name, source)
> 
> Here and below, could you please annotate this?

none of the methods of this class are annotated, so it does not make sense to annotate any particular one. we should annotate all of them (in a separate bug/patch :))

>> Source/WebCore/inspector/InjectedScriptSource.js:425
>> +            return "A function was expected.";
> 
> Either throw or log error and return undefined?

all other methods around would return a string on error, and none would throw an exception. let's be consistent.

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:74
>> +        this._args = Array.prototype.slice.call(this._args, 0);
> 
> Any reasons not to do this in the constructor?

optimization. yes, it may be premature, but the idea is to interfere as less as possible with the inspected page. we could even call freeze() from the constructor, but we won't need to do it for *every* call we capture. and there are going to be many many calls in a WebGL app, so this optimization makes sense to me, as for now.

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

these are the calls that contribute to the resource's state. they will be used in a replay.

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:155
>> +            this._resourceManager = new ResourceTrackingManager();
> 
> So a resource may implicitly get a manager different from others?

generally, there will be only one manager for a WebGL app resources. this is to provide info whether we are capturing now or not, as well as a callback for collecting the trace log.

>> 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?

"call" constructs a Call object, "result" returns an original result object of the call invocation. are these confusing names or what do you mean?

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

there is no "keys" getter defined on the glContext

>> 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.

done.

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:507
>> +            var name = object.constructor + "";
> 
> use toString()?

removed for now.

[Andrey Kosyakov]

Comment on attachment 154066 [details]
Patch

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

>>> Source/WebCore/inspector/InjectedScriptSource.js:420
>>>      injectModule: function(name, source)
>> 
>> Here and below, could you please annotate this?
> 
> none of the methods of this class are annotated, so it does not make sense to annotate any particular one. we should annotate all of them (in a separate bug/patch :))

grep -A 3 '/\*\*' Source/WebCore/inspector/InjectedScriptSource.js

>>> 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?
> 
> "call" constructs a Call object, "result" returns an original result object of the call invocation. are these confusing names or what do you mean?

My point is that you iterate through the properties of glContxt and check the name of its properties against WebGLRenderingContextResource.WrapFunction to see if custom wrap function is needed -- this seems a bit fragile, considering we have some internal properties there that are not intended to act as wrappers.

>>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:306
>>> +            processProperty(property);
>> 
>> glContext.keys.forEach(processProperty)?
> 
> there is no "keys" getter defined on the glContext

Duh, s/glContext.keys/Object.keys/, of course.

[Andrey Adaikin]

Comment on attachment 154066 [details]
Patch

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

>>>> Source/WebCore/inspector/InjectedScriptSource.js:420
>>>>      injectModule: function(name, source)
>>> 
>>> Here and below, could you please annotate this?
>> 
>> none of the methods of this class are annotated, so it does not make sense to annotate any particular one. we should annotate all of them (in a separate bug/patch :))
> 
> grep -A 3 '/\*\*' Source/WebCore/inspector/InjectedScriptSource.js

done.

>>> Source/WebCore/inspector/InjectedScriptSource.js:425
>>> +            return "A function was expected.";
>> 
>> Either throw or log error and return undefined?
> 
> all other methods around would return a string on error, and none would throw an exception. let's be consistent.

using console.error

>>>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:306
>>>> +            processProperty(property);
>>> 
>>> glContext.keys.forEach(processProperty)?
>> 
>> there is no "keys" getter defined on the glContext
> 
> Duh, s/glContext.keys/Object.keys/, of course.

Not quite what I need.
>Object.keys Returns an array of all own enumerable properties found upon a given object, in the same order as that provided by a for-in loop (the difference being that a for-in loop enumerates properties in the prototype chain as well).

[Andrey Adaikin]

Created attachment 154344 [details]
Patch

[Andrey Adaikin]

Created attachment 155247 [details]
less code

[Andrey Adaikin]

Created attachment 155277 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 155277 [details]
Patch

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

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:75
> +    freeze: function()
> +    {
> +        if (this._freezed)
> +            return;
> +        this._freezed = true;

This looks like a no-op -- can we nuke it for now?

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:181
> +            if (typeof glContext[property] === 'function') {

please use double quotes on strings

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:190
> +            } else

else {
   ...
}

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:208
>          proxy.__proto__ = glContext.__proto__;
>          proxy.constructor = glContext.constructor;

Do we need to set constructor explicitly, given that we set __proto__?

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:362
> +    reportBeforeCall: function(resource, functionName, args)

captureArguments? Also, do we need functionName?

[Andrey Adaikin]

Comment on attachment 155277 [details]
Patch

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

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:75
>> +        this._freezed = true;
> 
> This looks like a no-op -- can we nuke it for now?

nuked

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:181
>> +            if (typeof glContext[property] === 'function') {
> 
> please use double quotes on strings

done.

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:190
>> +            } else
> 
> else {
>    ...
> }

done.

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:208
>>          proxy.constructor = glContext.constructor;
> 
> Do we need to set constructor explicitly, given that we set __proto__?

removed.

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:362
>> +    reportBeforeCall: function(resource, functionName, args)
> 
> captureArguments? Also, do we need functionName?

renamed to captureArguments

[Andrey Adaikin]

Created attachment 155286 [details]
Patch

[Andrey Adaikin]

Created attachment 159366 [details]
Patch

[Pavel Feldman]

Comment on attachment 159366 [details]
Patch

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?

[Andrey Adaikin]

Comment on attachment 159366 [details]
Patch

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?

Yes, should never happen. We already call "inspectedWindow.console.error" in another place from this file. Do you want me to remove it?

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:39
>> +var Cache = function()
> 
> function Cache()
> {
> ...
> 
> Please annotate and add compilation phase into the compile-front-end.py

done.

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:45
>> +    get size()
> 
> Avoid getters for better compilability.
> size: function()
> {
> ...
> }

done.

>> 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?

we cannot use utilities.js here, I do use Object.create(null), Object.keys().length seems ineffective here.

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:75
>> + * @param {Array|Arguments} args
> 
> Please use alphabetical order for @ annotations. (@constructor goes first here)

done.

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

done.

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:122
>> +    if (!obj || obj instanceof Resource)
> 
> When does this happen? (the instanceof Resource case)

often, it's a legitimate case. although this case may not happen yet in this code.

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:143
>> +        this._wrappedObject = value;
> 
> Can we pass this in the constructor?

later we will need this setter

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

when a new WebGL resource is created that we should keep the record of

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

done.

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:193
>> +        function processProperty(property) {
> 
> { should go on the next line.

done.

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

done.

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:224
>> +            if (!manager || !manager.capturing)
> 
> When does it have no manager?

when we will be replaying, there should be no manager for the WebGL objects in the "replay world".

[Andrey Adaikin]

Created attachment 159434 [details]
Patch

[Pavel Feldman]

Comment on attachment 159434 [details]
Patch

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

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:164
> +};

drop the ; from all over the place plz.

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:369
> +    captureResource: function(resource)

Missing annotations.

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:374
> +    addCall: function(call)

Missing annotations.

[Andrey Adaikin]

Comment on attachment 159434 [details]
Patch

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

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:164
>> +};
> 
> drop the ; from all over the place plz.

done.

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:369
>> +    captureResource: function(resource)
> 
> Missing annotations.

done.

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:374
>> +    addCall: function(call)
> 
> Missing annotations.

done.

[Andrey Adaikin]

Created attachment 159457 [details]
Patch

[Pavel Feldman]

Comment on attachment 159457 [details]
Patch

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

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:138
> +};

Remove ;

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:225
> +};

Remove ;

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:307
> +};

Remove ;

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:349
> +};

Remove ;

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:486
>  };

Remove ;

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:513
> +};

Please remove ;

[Andrey Adaikin]

Created attachment 159675 [details]
Patch

[WebKit Review Bot]

Comment on attachment 159675 [details]
Patch

Rejecting attachment 159675 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
pector/InjectedScriptSource.js
Hunk #1 FAILED at 554.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/inspector/InjectedScriptSource.js.rej
patching file Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js
patching file Source/WebCore/inspector/compile-front-end.py
Hunk #1 succeeded at 377 (offset -3 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Pavel Feld..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13564041

[Andrey Adaikin]

Created attachment 159878 [details]
Rebased patch to land

[Andrey Kosyakov]

Committed r126283: <http://trac.webkit.org/changeset/126283>