Bug 95443 – Web Inspector: [WebGL] Save gl.getError() status before taking a WebGL state snapshot and restore it afterwards

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Andrey Adaikin]

Patch to follow

[Andrey Adaikin]

Created attachment 161451 [details]
Patch

[Vsevolod Vlasov]

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?

[Andrey Adaikin]

> > 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

[Andrey Adaikin]

Created attachment 161644 [details]
Rebased and reduced patch

[Vsevolod Vlasov]

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?

[Vsevolod Vlasov]

Comment on attachment 161644 [details]
Rebased and reduced patch

How can we test this?

[Andrey Adaikin]

Created attachment 161712 [details]
Patch

[Vsevolod Vlasov]

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.

[Vsevolod Vlasov]

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.

[Andrey Adaikin]

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

[Andrey Adaikin]

Created attachment 161984 [details]
Patch

[Andrey Adaikin]

Created attachment 161987 [details]
+key -> Number(key)

[WebKit Review Bot]

Comment on attachment 161987 [details]
+key -> Number(key)

Clearing flags on attachment: 161987

Committed r127452: <http://trac.webkit.org/changeset/127452>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.