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.
Created attachment 147543 [details] Patch
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?
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).
Created attachment 147570 [details] Patch
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?
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.
Created attachment 147577 [details] Patch
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?
(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
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(...)
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?
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.
Comment on attachment 147577 [details] Patch Clearing flags on attachment: 147577 Committed r120421: <http://trac.webkit.org/changeset/120421>
All reviewed patches have been landed. Closing bug.