Bug 89088 – Web Inspector: [WebGL] Simple implementation of the InjectedWebGLScriptSource to support capturing WebGL calls for a frame

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

[Andrey Adaikin]

Simple experimental implementation of the InjectedWebGLScriptSource.js that allows to wrap a WebGL context and capture names (for now) of the WebGL function calls for a frame being captured.

[Andrey Adaikin]

Created attachment 147543 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 147543 [details]
Patch

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

> Source/WebCore/inspector/InjectedWebGLScriptSource.js:41
> +    this._lastBoundObjectId = 1;

We use 0 as initial "last" value and prefix increment. Otherwise the proper name would be "nextBoundObjectId".

> Source/WebCore/inspector/InjectedWebGLScriptSource.js:58
> +        var nameProcessed = {};
> +        nameProcessed["__proto__"] = null;
> +        nameProcessed["constructor"] = true;

Why not use object literal for that?

> Source/WebCore/inspector/InjectedWebGLScriptSource.js:60
> +            Object.getOwnPropertyNames(o).forEach(function(name) {

Please split function definition from usage. forEach(function() { ... /* 15 lines */ }.bind(...)) is not readable. This can probably be outside of the loop.

> Source/WebCore/inspector/InjectedWebGLScriptSource.js:66
> +                if (typeof glContext[name] == "function") {
> +                    proxy[name] = this._wrappedFunction.bind(this, glContext, name);
> +                } else {

braces are redundant.

> Source/WebCore/inspector/InjectedWebGLScriptSource.js:81
> +        proxy["__proto__"] = glContext["__proto__"];
> +        proxy["constructor"] = glContext["constructor"];

why not name.field instead of name["field"]?

> Source/WebCore/inspector/InjectedWebGLScriptSource.js:93
> +        var id = this._lastBoundObjectId++;

As above -- look more like nextBoundObjectId.

> Source/WebCore/inspector/InjectedWebGLScriptSource.js:112
> +    _wrappedFunction: function(glContext, functionName, var_args) {

style: { => new line. also, wrt 'var' -- does this compile?

> Source/WebCore/inspector/InjectedWebGLScriptSource.js:121
> +            if (capturedCallsNum == 1) {
> +                this._setZeroTimeouts(this._stopCapturing.bind(this, this._capturingFrameInfo));
> +            }

Braces are redundant.

> Source/WebCore/inspector/InjectedWebGLScriptSource.js:131
> +        var channel = new MessageChannel();
> +        channel.port1.onmessage = callback;
> +        channel.port2.postMessage("");

What is this for?

[Andrey Adaikin]

Comment on attachment 147543 [details]
Patch

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

>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:41
>> +    this._lastBoundObjectId = 1;
> 
> We use 0 as initial "last" value and prefix increment. Otherwise the proper name would be "nextBoundObjectId".

Done.

>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:58
>> +        nameProcessed["constructor"] = true;
> 
> Why not use object literal for that?

Done.

>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:60
>> +            Object.getOwnPropertyNames(o).forEach(function(name) {
> 
> Please split function definition from usage. forEach(function() { ... /* 15 lines */ }.bind(...)) is not readable. This can probably be outside of the loop.

Done.

>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:66
>> +                } else {
> 
> braces are redundant.

Done.

>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:81
>> +        proxy["constructor"] = glContext["constructor"];
> 
> why not name.field instead of name["field"]?

Done.

>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:93
>> +        var id = this._lastBoundObjectId++;
> 
> As above -- look more like nextBoundObjectId.

Done.

>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:112
>> +    _wrappedFunction: function(glContext, functionName, var_args) {
> 
> style: { => new line. also, wrt 'var' -- does this compile?

Done. Removed var_args.

>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:121
>> +            }
> 
> Braces are redundant.

Done.

>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:131
>> +        channel.port2.postMessage("");
> 
> What is this for?

This is to emulate the "end frame" event. Later we may want some help here from the backend.
Added comments:
        // We need a fastest async callback, whatever fires first.
        // Usually a postMessage should be faster than a setTimeout(0).

[Andrey Adaikin]

Created attachment 147570 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 147570 [details]
Patch

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

> Source/WebCore/inspector/InjectedWebGLScriptSource.js:57
> +        nameProcessed.__proto__ = null;

what's that for?

> Source/WebCore/inspector/InjectedWebGLScriptSource.js:64
> +            if (typeof glContext[name] == "function")

== => ===

> Source/WebCore/inspector/InjectedWebGLScriptSource.js:68
> +                    get: function() {

{ => next line

> Source/WebCore/inspector/InjectedWebGLScriptSource.js:71
> +                    set: function(value) {

ditto

> Source/WebCore/inspector/InjectedWebGLScriptSource.js:121
> +            if (capturedCallsNum == 1)

== => ===

> Source/WebCore/inspector/InjectedWebGLScriptSource.js:123
> +            InjectedScriptHost.webGLReportFunctionCall(this._capturingFrameInfo.contextId, functionName, "[" + args.join(", ") + "]", result + "");

", " => ",", let's be cheap :-)
result + "" => result.toString()? Also, what if a call returns object?

[Andrey Adaikin]

Comment on attachment 147570 [details]
Patch

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

>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:57
>> +        nameProcessed.__proto__ = null;
> 
> what's that for?

so that nameProcessed.constructor, nameProcessed.toString and etc. would be undefined. we do this in the InjectedScriptSource.js also.

>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:64
>> +            if (typeof glContext[name] == "function")
> 
> == => ===

Done.

>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:68
>> +                    get: function() {
> 
> { => next line

Done.

>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:71
>> +                    set: function(value) {
> 
> ditto

Done.

>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:121
>> +            if (capturedCallsNum == 1)
> 
> == => ===

Done.

>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:123
>> +            InjectedScriptHost.webGLReportFunctionCall(this._capturingFrameInfo.contextId, functionName, "[" + args.join(", ") + "]", result + "");
> 
> ", " => ",", let's be cheap :-)
> result + "" => result.toString()? Also, what if a call returns object?

By all means, this is only a temporary solution, as we will need to serialize actual arguments and transport them to the front-end in order to be able to "replay". Right now, for a first demo, we just want to display the names of the functions, with "stubbed" arguments and result.

[Andrey Adaikin]

Created attachment 147577 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 147577 [details]
Patch

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

LGTM with one nit. Pavel?

> Source/WebCore/inspector/InjectedWebGLScriptSource.js:57
> +        nameProcessed.__proto__ = null;

Does this have any effect?

[Andrey Kosyakov]

(In reply to comment #8)
> (From update of attachment 147577 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147577&action=review
> 
> LGTM with one nit. Pavel?
> 
> > Source/WebCore/inspector/InjectedWebGLScriptSource.js:57
> > +        nameProcessed.__proto__ = null;
> 
> Does this have any effect?

Ah, I see the intent. I think it would be better to use Object.hasOwnProperty when checking for property to be in nameProcessed

[Andrey Adaikin]

Comment on attachment 147577 [details]
Patch

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

>>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:57
>>> +        nameProcessed.__proto__ = null;
>> 
>> Does this have any effect?
> 
> Ah, I see the intent. I think it would be better to use Object.hasOwnProperty when checking for property to be in nameProcessed

We will execute nameProcessed["hasOwnProperty"]=true at some point :)
If we get rid of nameProcessed and use proxy.hasOwnProperty() instead, the same problem will arize after executing proxy.hasOwnProperty = _wrappedFunction(...)

[Vsevolod Vlasov]

Comment on attachment 147577 [details]
Patch

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

>>>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:57
>>>> +        nameProcessed.__proto__ = null;
>>> 
>>> Does this have any effect?
>> 
>> Ah, I see the intent. I think it would be better to use Object.hasOwnProperty when checking for property to be in nameProcessed
> 
> We will execute nameProcessed["hasOwnProperty"]=true at some point :)
> If we get rid of nameProcessed and use proxy.hasOwnProperty() instead, the same problem will arize after executing proxy.hasOwnProperty = _wrappedFunction(...)

Consider using prefix for storing in nameProcessed or Object.getOwnPropertyDescriptor(object, propertyName)

> Source/WebCore/inspector/InjectedWebGLScriptSource.js:102
> +    {

Shouldn't we check that we are not capturing already here?

[Andrey Adaikin]

Comment on attachment 147577 [details]
Patch

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

>>>>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:57
>>>>> +        nameProcessed.__proto__ = null;
>>>> 
>>>> Does this have any effect?
>>> 
>>> Ah, I see the intent. I think it would be better to use Object.hasOwnProperty when checking for property to be in nameProcessed
>> 
>> We will execute nameProcessed["hasOwnProperty"]=true at some point :)
>> If we get rid of nameProcessed and use proxy.hasOwnProperty() instead, the same problem will arize after executing proxy.hasOwnProperty = _wrappedFunction(...)
> 
> Consider using prefix for storing in nameProcessed or Object.getOwnPropertyDescriptor(object, propertyName)

Yes, using prefixes is an option, and __proto__=null is another. I like the latter one, given that we already use this method in the InjectedScriptSource.js

>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:102
>> +    {
> 
> Shouldn't we check that we are not capturing already here?

Not for now. If we add a frame-ending event to the protocol, then we will have to notify the frontend from here.

[WebKit Review Bot]

Comment on attachment 147577 [details]
Patch

Clearing flags on attachment: 147577

Committed r120421: <http://trac.webkit.org/changeset/120421>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.