WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
89088
Web Inspector: [WebGL] Simple implementation of the InjectedWebGLScriptSource to support capturing WebGL calls for a frame
https://bugs.webkit.org/show_bug.cgi?id=89088
Summary
Web Inspector: [WebGL] Simple implementation of the InjectedWebGLScriptSource...
Andrey Adaikin
Reported
2012-06-14 03:19:37 PDT
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.
Attachments
Patch
(5.00 KB, patch)
2012-06-14 03:22 PDT
,
Andrey Adaikin
no flags
Details
Formatted Diff
Diff
Patch
(5.05 KB, patch)
2012-06-14 06:26 PDT
,
Andrey Adaikin
no flags
Details
Formatted Diff
Diff
Patch
(5.10 KB, patch)
2012-06-14 06:55 PDT
,
Andrey Adaikin
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Andrey Adaikin
Comment 1
2012-06-14 03:22:25 PDT
Created
attachment 147543
[details]
Patch
Andrey Kosyakov
Comment 2
2012-06-14 06:09:20 PDT
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 3
2012-06-14 06:26:18 PDT
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
Comment 4
2012-06-14 06:26:58 PDT
Created
attachment 147570
[details]
Patch
Andrey Kosyakov
Comment 5
2012-06-14 06:40:28 PDT
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 6
2012-06-14 06:55:18 PDT
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
Comment 7
2012-06-14 06:55:43 PDT
Created
attachment 147577
[details]
Patch
Andrey Kosyakov
Comment 8
2012-06-14 06:56:51 PDT
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
Comment 9
2012-06-14 07:00:01 PDT
(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 10
2012-06-14 07:07:36 PDT
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 11
2012-06-14 07:39:40 PDT
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 12
2012-06-14 08:10:03 PDT
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 13
2012-06-15 01:16:02 PDT
Comment on
attachment 147577
[details]
Patch Clearing flags on attachment: 147577 Committed
r120421
: <
http://trac.webkit.org/changeset/120421
>
WebKit Review Bot
Comment 14
2012-06-15 01:16:07 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug