Bug 95443

Summary: Web Inspector: [WebGL] Save gl.getError() status before taking a WebGL state snapshot and restore it afterwards
Product: WebKit Reporter: Andrey Adaikin <aandrey>
Component: Web Inspector (Deprecated)Assignee: Andrey Adaikin <aandrey>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, benvanik, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 95533    
Bug Blocks: 95733    
Attachments:
Description Flags
Patch
none
Rebased and reduced patch
none
Patch
none
Patch
none
+key -> Number(key) none

Description Andrey Adaikin 2012-08-30 04:24:48 PDT
Patch to follow
Comment 1 Andrey Adaikin 2012-08-30 05:40:54 PDT
Created attachment 161451 [details]
Patch
Comment 2 Vsevolod Vlasov 2012-08-30 08:06:28 PDT
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?
Comment 3 Andrey Adaikin 2012-08-31 00:47:16 PDT
> > 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
Comment 4 Andrey Adaikin 2012-08-31 02:29:45 PDT
Created attachment 161644 [details]
Rebased and reduced patch
Comment 5 Vsevolod Vlasov 2012-08-31 04:21:29 PDT
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 6 Vsevolod Vlasov 2012-08-31 05:08:43 PDT
Comment on attachment 161644 [details]
Rebased and reduced patch

How can we test this?
Comment 7 Andrey Adaikin 2012-08-31 09:06:36 PDT
Created attachment 161712 [details]
Patch
Comment 8 Vsevolod Vlasov 2012-09-03 10:40:23 PDT
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 9 Vsevolod Vlasov 2012-09-03 10:45:19 PDT
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 10 Andrey Adaikin 2012-09-04 01:10:14 PDT
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
Comment 11 Andrey Adaikin 2012-09-04 01:12:49 PDT
Created attachment 161984 [details]
Patch
Comment 12 Andrey Adaikin 2012-09-04 01:41:45 PDT
Created attachment 161987 [details]
+key -> Number(key)
Comment 13 WebKit Review Bot 2012-09-04 02:37:17 PDT
Comment on attachment 161987 [details]
+key -> Number(key)

Clearing flags on attachment: 161987

Committed r127452: <http://trac.webkit.org/changeset/127452>
Comment 14 WebKit Review Bot 2012-09-04 02:37:21 PDT
All reviewed patches have been landed.  Closing bug.