Bug 94696 – Web Inspector: [WebGL] Simple experimental frontend implementation

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

[Andrey Adaikin]

Just a simple frontend to start with.
Patch and a screenshot to follow.

[Andrey Adaikin]

Created attachment 159914 [details]
Screenshot

[Andrey Adaikin]

Created attachment 159915 [details]
Patch

[Andrey Adaikin]

Created attachment 159917 [details]
Patch

[Andrey Adaikin]

Created attachment 159919 [details]
Screenshot 2

[Andrey Kosyakov]

Comment on attachment 159917 [details]
Patch

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

> Source/WebCore/inspector/front-end/WebGLPanel.js:88
> +    wasShown: function()
> +    {
> +        WebInspector.Panel.prototype.wasShown.call(this);
> +    },
> +
> +    willHide: function()
> +    {
> +        WebInspector.Panel.prototype.willHide.call(this);
> +    },
> +

Redundant?

> Source/WebCore/inspector/front-end/WebGLPanel.js:106
> +        WebInspector.webGLManager.captureFrame(function(traceLogId) {
> +            var elm = document.createElement("div");
> +            elm.textContent = traceLogId;
> +            elm.traceLogId = traceLogId;
> +            elm.addEventListener("click", this._onTraceLogListItemClick.bind(this), false);
> +            this._traceLogsListElement.appendChild(elm);
> +        }.bind(this));

Please extract the function.

> Source/WebCore/inspector/front-end/WebGLPanel.js:112
> +        WebInspector.webGLManager.getTraceLog(listItem.traceLogId, function(traceLog) {

ditto.

> Source/WebCore/inspector/front-end/WebGLPanel.js:136
> +        WebInspector.webGLManager.replayTraceLog(elm.traceLogId, elm.stepNo, function(dataUrl) {

ditto

> Source/WebCore/inspector/front-end/WebGLPanel.js:168
> +        this._enablementCount++;
> +        if (this._enablementCount === 1)

I'd swap these two statements and checked for !this._enablementCount

[Pavel Feldman]

Comment on attachment 159917 [details]
Patch

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

> Source/WebCore/WebCore.gypi:6389
> +            'inspector/front-end/WebGLPanel.js',

This panel should not be a part of the core, please load WebGLPanel as a module (lazily).

> Source/WebCore/inspector/front-end/inspector.html:161
> +    <script type="text/javascript" src="WebGLPanel.js"></script>

Should be a module.

[Andrey Adaikin]

Comment on attachment 159917 [details]
Patch

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

>> Source/WebCore/inspector/front-end/WebGLPanel.js:88
>> +
> 
> Redundant?

removed

>> Source/WebCore/inspector/front-end/WebGLPanel.js:106
>> +        }.bind(this));
> 
> Please extract the function.

done

>> Source/WebCore/inspector/front-end/WebGLPanel.js:112
>> +        WebInspector.webGLManager.getTraceLog(listItem.traceLogId, function(traceLog) {
> 
> ditto.

done

>> Source/WebCore/inspector/front-end/WebGLPanel.js:136
>> +        WebInspector.webGLManager.replayTraceLog(elm.traceLogId, elm.stepNo, function(dataUrl) {
> 
> ditto

done

>> Source/WebCore/inspector/front-end/WebGLPanel.js:168
>> +        if (this._enablementCount === 1)
> 
> I'd swap these two statements and checked for !this._enablementCount

FYI, I copied this from WebInspector.TimelineManager class.
And I like this way better, because it's safer - someone may call start() from within a callback of WebGLAgent.enable (i.e. from an event listener handler of WebInspector.WebGLManager.EventTypes.WebGLStarted)

>> Source/WebCore/inspector/front-end/inspector.html:161
>> +    <script type="text/javascript" src="WebGLPanel.js"></script>
> 
> Should be a module.

okay...

[Andrey Adaikin]

Created attachment 159950 [details]
Patch

[Pavel Feldman]

As we agreed offline, lets make it a profile type.

[Andrey Adaikin]

Work is in progress...

[Andrey Adaikin]

Created attachment 160397 [details]
Made it a profile type

[Andrey Adaikin]

Created attachment 160399 [details]
Screenshot, WebGL profile type

[Vsevolod Vlasov]

Comment on attachment 160397 [details]
Made it a profile type

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

> Source/WebCore/inspector/front-end/WebGLProfileView.js:103
> +            for (var i = 0, n = calls.length; i < n; ++i) {

for (var i = 0; i < calls.length; ++i) ?

> Source/WebCore/inspector/front-end/WebGLProfileView.js:105
> +                var elm = document.createElement("div");

traceLogItem

> Source/WebCore/inspector/front-end/WebGLProfileView.js:121
> +        function didReplayTraceLog(error, dataUrl)

dataURL

[Andrey Adaikin]

Created attachment 160717 [details]
Patch to land

[Andrey Kosyakov]

Committed r126855: <http://trac.webkit.org/changeset/126855>