| Summary: | Web Inspector: [WebGL] Generic framework draft for tracking WebGL resources | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Andrey Adaikin <aandrey> | ||||||||||||||||||||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Andrey Adaikin <aandrey> | ||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
| Severity: | Normal | CC: | apavlov, benvanik, bweinstein, caseq, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||
|
Description
Andrey Adaikin
2012-07-05 03:04:58 PDT
Created attachment 150911 [details]
Work in progress
Created attachment 154066 [details]
Patch
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. 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 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. 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. 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). Created attachment 154344 [details]
Patch
Created attachment 155247 [details]
less code
Created attachment 155277 [details]
Patch
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? 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 Created attachment 155286 [details]
Patch
Created attachment 159366 [details]
Patch
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? 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". Created attachment 159434 [details]
Patch
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. 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. Created attachment 159457 [details]
Patch
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 ; Created attachment 159675 [details]
Patch
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 Created attachment 159878 [details]
Rebased patch to land
Committed r126283: <http://trac.webkit.org/changeset/126283> |