Patch to follow
Created attachment 161451 [details] Patch
Comment on attachment 161451 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161451&action=review > Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:43 > + TypedArrayClasses: (function(typeNames) { Why did this change? > Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:1529 > + var wrapFunctions = WebGLRenderingContextResource._WrapFunctions; Can we make wrapFunctions changes in a separate patch?
> > Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:1529 > > + var wrapFunctions = WebGLRenderingContextResource._WrapFunctions; > > Can we make wrapFunctions changes in a separate patch? Moved this to https://bugs.webkit.org/show_bug.cgi?id=95533
Created attachment 161644 [details] Rebased and reduced patch
Comment on attachment 161644 [details] Rebased and reduced patch View in context: https://bugs.webkit.org/attachment.cgi?id=161644&action=review > Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:-1113 > - // FIXME: Call while(gl.getError() != gl.NO_ERROR) {...} to check if a particular parameter is supported. Why did this change?
Comment on attachment 161644 [details] Rebased and reduced patch How can we test this?
Created attachment 161712 [details] Patch
Comment on attachment 161712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161712&action=review > Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:1123 > + Object.keys(this._customErrors).forEach(function(key) { Please extract this function and give it a name. > Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:1124 > + var error = +key; +key? > Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:1127 > + this._customErrors = null; delete this._customErrors; > Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:1145 > + this._customErrors = null; delete this._customErrors; > Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:1147 > + this._customErrors = Object.create(null); this._customErrors = {}; > Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:1174 > + this._customErrors = null; delete this._customErrors; > LayoutTests/inspector/profiler/webgl/webgl-profiler-get-error.html:23 > + a.forEach(function(elm, index) { elm -> element > LayoutTests/inspector/profiler/webgl/webgl-profiler-get-error.html:77 > + errors.forEach(function(error) { I don't think this one is needed, we should not verify WebGL spec here. > LayoutTests/inspector/profiler/webgl/webgl-profiler-get-error.html:129 > + InspectorTest.runTestSuite([ Why do you need runTestSuite? > LayoutTests/inspector/profiler/webgl/webgl-profiler-get-error.html:149 > +<p> Please add a link to the bug here.
Comment on attachment 161712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161712&action=review > LayoutTests/inspector/profiler/webgl/webgl-profiler-get-error.html:64 > + rawgl = glResource.wrappedObject(); rawGL > LayoutTests/inspector/profiler/webgl/webgl-profiler-get-error.html:74 > + errors.forEach(function(error) { We prefer not to inline functions. Maybe extract generateWebGLErrors instead? > LayoutTests/inspector/profiler/webgl/webgl-profiler-get-error.html:135 > + InspectorTest.evaluateInConsole("createAndRunWebGLProgram()", step2); I would replace step2 with next since you are just asserting that the function ended there.
Comment on attachment 161712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161712&action=review >> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:1123 >> + Object.keys(this._customErrors).forEach(function(key) { > > Please extract this function and give it a name. Replaced with for..in loop >> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:1124 >> + var error = +key; > > +key? var error = +key; // Convert to Number. >> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:1127 >> + this._customErrors = null; > > delete this._customErrors; done >> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:1145 >> + this._customErrors = null; > > delete this._customErrors; done >> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:1147 >> + this._customErrors = Object.create(null); > > this._customErrors = {}; done >> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:1174 >> + this._customErrors = null; > > delete this._customErrors; done >> LayoutTests/inspector/profiler/webgl/webgl-profiler-get-error.html:23 >> + a.forEach(function(elm, index) { > > elm -> element done >> LayoutTests/inspector/profiler/webgl/webgl-profiler-get-error.html:64 >> + rawgl = glResource.wrappedObject(); > > rawGL done >> LayoutTests/inspector/profiler/webgl/webgl-profiler-get-error.html:74 >> + errors.forEach(function(error) { > > We prefer not to inline functions. Maybe extract generateWebGLErrors instead? errors.forEach(generateWebGLError.bind(this, rawGL)); >> LayoutTests/inspector/profiler/webgl/webgl-profiler-get-error.html:77 >> + errors.forEach(function(error) { > > I don't think this one is needed, we should not verify WebGL spec here. removed. >> LayoutTests/inspector/profiler/webgl/webgl-profiler-get-error.html:129 >> + InspectorTest.runTestSuite([ > > Why do you need runTestSuite? removed >> LayoutTests/inspector/profiler/webgl/webgl-profiler-get-error.html:135 >> + InspectorTest.evaluateInConsole("createAndRunWebGLProgram()", step2); > > I would replace step2 with next since you are just asserting that the function ended there. I need to check the return value also (asynchronously) - this will mean there were no exceptions thrown. >> LayoutTests/inspector/profiler/webgl/webgl-profiler-get-error.html:149 >> +<p> > > Please add a link to the bug here. done
Created attachment 161984 [details] Patch
Created attachment 161987 [details] +key -> Number(key)
Comment on attachment 161987 [details] +key -> Number(key) Clearing flags on attachment: 161987 Committed r127452: <http://trac.webkit.org/changeset/127452>
All reviewed patches have been landed. Closing bug.