Bug 111162 – Web Inspector: implement Flame Chart for CPU profiler

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

[Ilya Tikhonovsky]

Flame Chart may give to the developer a better clue what is going on with the performance without expanding the entire tree.

http://dtrace.org/blogs/brendan/2011/12/16/flame-graphs/

[Ilya Tikhonovsky]

Created attachment 190933 [details]
screenshot

[Ilya Tikhonovsky]

Created attachment 190934 [details]
Patch

[Ilya Tikhonovsky]

Created attachment 190938 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 190938 [details]
Patch

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

> Source/WebCore/inspector/front-end/CPUProfileView.js:65
> +    } else {

style: no braces for one-line block.

> Source/WebCore/inspector/front-end/CPUProfileView.js:101
> +WebInspector.FlameChart = function(cpuProfileView)

Why not move it into its own file?

> Source/WebCore/inspector/front-end/CPUProfileView.js:107
> +    this._canvas = document.createElement("canvas");

this._canvas = this.element.createChild("canvas");

> Source/WebCore/inspector/front-end/CPUProfileView.js:183
> +    draw: function(height, width)

It is usually width, heigh (in reverse order).

> Source/WebCore/inspector/front-end/CPUProfileView.js:194
> +        this._color = initialColor;

Consider using WebInspector.Color

> Source/WebCore/inspector/front-end/CPUProfileView.js:211
> +    _forEach: function(callback)

Please rename _forEach -> _forEachNode

> Source/WebCore/inspector/front-end/CPUProfileView.js:227
> +                    nodes = nodes.concat(node.children);

This way the nodes will appear in reverse order, won't they?

> Source/WebCore/inspector/front-end/CPUProfileView.js:241
> +     * @param {!ProfilerAgent.CPUProfileNode} node

color annotation is missing, also I'd rename it to rgbColor

> Source/WebCore/inspector/front-end/CPUProfileView.js:900
> +

Revert this?

> Source/WebCore/inspector/front-end/CPUProfileView.js:934
> +//@ sourceURL=http://localhost/inspector/front-end/CPUProfileView.js

Remove this.

[Ilya Tikhonovsky]

Created attachment 191199 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 190938 [details]
Patch

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

>> Source/WebCore/inspector/front-end/CPUProfileView.js:194
>> +        this._color = initialColor;
> 
> Consider using WebInspector.Color

done

>> Source/WebCore/inspector/front-end/CPUProfileView.js:211
>> +    _forEach: function(callback)
> 
> Please rename _forEach -> _forEachNode

done

>> Source/WebCore/inspector/front-end/CPUProfileView.js:227
>> +                    nodes = nodes.concat(node.children);
> 
> This way the nodes will appear in reverse order, won't they?

It is not a problem because children have no order.

>> Source/WebCore/inspector/front-end/CPUProfileView.js:241
>> +     * @param {!ProfilerAgent.CPUProfileNode} node
> 
> color annotation is missing, also I'd rename it to rgbColor

done: hslColor

>> Source/WebCore/inspector/front-end/CPUProfileView.js:900
>> +
> 
> Revert this?

done

>> Source/WebCore/inspector/front-end/CPUProfileView.js:934
>> +//@ sourceURL=http://localhost/inspector/front-end/CPUProfileView.js
> 
> Remove this.

done

[Ilya Tikhonovsky]

Created attachment 191200 [details]
Patch

[Ilya Tikhonovsky]

Committed r144618: <http://trac.webkit.org/changeset/144618>