Bug 94686 – Web Inspector: [WebGL] Allow collecting calls for Resource objects affecting their states

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

[Andrey Adaikin]

Next step in WebGL instrumentation.

[Andrey Adaikin]

Created attachment 159893 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 159893 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159893&action=review

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:43
> +    TYPED_ARRAY_CLASSES: (function(typeNames) {

I'd split it into isSupportedTypedArray() and ["Int8Array", ...].filter(isSupportedTypedArray)

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:79
> +        var t = typeof obj;
> +        if (t !== "object" && t !== "function")
> +            return obj;

t => type? or perhaps replace with:
if (!(obj instanceof Object))
    return obj;

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:83
> +        if (typeof obj.slice === "function")
> +            return obj.slice(0);

I wonder if we should have a more specific type check here?

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:438
> +            var wrapFunction = new WebGLRenderingContextResource.WrapFunction(originalObject, originalFunction, functionName, arguments);
> +            customWrapFunction.apply(wrapFunction, arguments);

Do we need a new instance for each invocation? Can we postpone implementing this until we have actual custom wrap functions?

[Pavel Feldman]

Comment on attachment 159893 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159893&action=review

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:46
> +            if (window[typeNames[i]])

You should use inspectedWindow here.

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:58
> +        var classes = TypeUtils.TYPED_ARRAY_CLASSES;

WebKit uses camel case for constants.

> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:60
> +            if (array instanceof classes[i])

First check if InjectedScriptHost.type(array) ===  "array".

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:79
>> +            return obj;
> 
> t => type? or perhaps replace with:
> if (!(obj instanceof Object))
>     return obj;

Always prefer typeof to instanceof.

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:83
>> +            return obj.slice(0);
> 
> I wonder if we should have a more specific type check here?

slice()

[Andrey Adaikin]

Comment on attachment 159893 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159893&action=review

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:43
>> +    TYPED_ARRAY_CLASSES: (function(typeNames) {
> 
> I'd split it into isSupportedTypedArray() and ["Int8Array", ...].filter(isSupportedTypedArray)

filter will result in the same args as in the original array (strings in this case), while we need to convert them to a Function

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:79
>> +            return obj;
> 
> t => type? or perhaps replace with:
> if (!(obj instanceof Object))
>     return obj;

the purpose is to filter out primitive types here. instanceof is not good:

a = Object.create(null)
>> Object
a instanceof Object
>> false
typeof a
>> "object"

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:83
>> +            return obj.slice(0);
> 
> I wonder if we should have a more specific type check here?

added console.assert(obj instanceof Array || obj instanceof ArrayBuffer);

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:438
>> +            customWrapFunction.apply(wrapFunction, arguments);
> 
> Do we need a new instance for each invocation? Can we postpone implementing this until we have actual custom wrap functions?

removed

[Andrey Adaikin]

Created attachment 159933 [details]
Patch

[Andrey Adaikin]

Comment on attachment 159893 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159893&action=review

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:46
>> +            if (window[typeNames[i]])
> 
> You should use inspectedWindow here.

done.

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:58
>> +        var classes = TypeUtils.TYPED_ARRAY_CLASSES;
> 
> WebKit uses camel case for constants.

done.

>> Source/WebCore/inspector/InjectedScriptWebGLModuleSource.js:60
>> +            if (array instanceof classes[i])
> 
> First check if InjectedScriptHost.type(array) ===  "array".

1) why?
2) I want to minimize dependency on Web Inspector backend. Right now the WebGL module can be run autonomously. I can accept using "inspectedWindow", but for InjectedScriptHost plz give some strong reasons.

[Andrey Adaikin]

Created attachment 159935 [details]
Patch

[Andrey Adaikin]

Created attachment 159951 [details]
Rebased

[Pavel Feldman]

> 1) why?

Because it will be a fast check and you'll only call instanceof for arrays.

> 2) I want to minimize dependency on Web Inspector backend. Right now the WebGL module can be run autonomously. I can accept using "inspectedWindow", but for InjectedScriptHost plz give some strong reasons.

Ok, I see. I don't see a big deal in using InjectedScriptHost given that your code is in the WebKit repo. I.e. when one needs to re-use it outside inspector, it is very easy to mock that InjectedScriptHost.

I think once you hit the production, you'll need to switch from instanceof calls to the proper native checks via InjectedScriptHost. That was our experience while working with InjectedScript. We started all JavaScript, but then realized that the more is implemented natively, the more reliable it is. Things were crashing upon navigation, weird things were happening when the code was called near the page navigation point. But feel free to leave it as is and we'll see whether it crashes.

[WebKit Review Bot]

Comment on attachment 159951 [details]
Rebased

Clearing flags on attachment: 159951

Committed r126413: <http://trac.webkit.org/changeset/126413>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.