Bug 111162

Summary: Web Inspector: implement Flame Chart for CPU profiler
Product: WebKit Reporter: Ilya Tikhonovsky <loislo>
Component: Web Inspector (Deprecated)Assignee: Ilya Tikhonovsky <loislo>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, keishi, loislo, pfeldman, pmuellr, vsevik, web-inspector-bugs, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
screenshot
none
Patch
none
Patch
none
Patch
none
Patch yurys: review+

Description Ilya Tikhonovsky 2013-03-01 04:33:26 PST 
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/
Comment 1 Ilya Tikhonovsky 2013-03-01 04:35:00 PST 
Created attachment 190933 [details]
screenshot
Comment 2 Ilya Tikhonovsky 2013-03-01 04:37:34 PST 
Created attachment 190934 [details]
Patch
Comment 3 Ilya Tikhonovsky 2013-03-01 04:49:57 PST 
Created attachment 190938 [details]
Patch
Comment 4 Yury Semikhatsky 2013-03-03 22:56:17 PST 
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.
Comment 5 Ilya Tikhonovsky 2013-03-04 04:00:22 PST 
Created attachment 191199 [details]
Patch
Comment 6 Ilya Tikhonovsky 2013-03-04 04:02:57 PST 
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
Comment 7 Ilya Tikhonovsky 2013-03-04 04:04:40 PST 
Created attachment 191200 [details]
Patch
Comment 8 Ilya Tikhonovsky 2013-03-04 04:41:57 PST 
Committed r144618: <http://trac.webkit.org/changeset/144618>