Bug 113410 – Web Inspector: Flame Chart. Developers will have more clue if two different profiles will have same colors for same functions.

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

[Ilya Tikhonovsky]

scenario:

developer made CPUProfile on a page.
developer fixed the problem
developer made another CPUProfile 
developer has to see the difference
but he might be confused by the different colors associated with the same function in the different profiles.

[Ilya Tikhonovsky]

Created attachment 195331 [details]
Patch

[Build Bot]

Comment on attachment 195331 [details]
Patch

Attachment 195331 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17294365

[Pavel Feldman]

Comment on attachment 195331 [details]
Patch

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

> Source/WebCore/inspector/front-end/FlameChart.js:74
> +    var profilesPanel = WebInspector.panels["profiles"];

FlameChart should not depend on the panel.

> Source/WebCore/inspector/front-end/FlameChart.js:77
> +        this._colorGenerator = profilesPanel.colorGenerator = new WebInspector.FlameChart.ColorGenerator();

A statement per line please.

[Ilya Tikhonovsky]

Created attachment 195549 [details]
Patch

[Ilya Tikhonovsky]

(In reply to comment #3)
> (From update of attachment 195331 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195331&action=review
> 
> > Source/WebCore/inspector/front-end/FlameChart.js:74
> > +    var profilesPanel = WebInspector.panels["profiles"];
> 
> FlameChart should not depend on the panel.

done

> 
> > Source/WebCore/inspector/front-end/FlameChart.js:77
> > +        this._colorGenerator = profilesPanel.colorGenerator = new WebInspector.FlameChart.ColorGenerator();
> 
> A statement per line please.

done

[Ilya Tikhonovsky]

Created attachment 195551 [details]
screenshot

[Pavel Feldman]

Comment on attachment 195549 [details]
Patch

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

> Source/WebCore/inspector/front-end/FlameChart.js:75
> +    this._colorGenerator = panel.colorGenerator;

You can't declare properties on alien objects, just make it static!

I.e. if (!WebInspector.FlameChart._colorGenerator)

[Ilya Tikhonovsky]

Created attachment 195729 [details]
Patch

[Pavel Feldman]

Comment on attachment 195729 [details]
Patch

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

> Source/WebCore/inspector/front-end/FlameChart.js:74
> +    this._colorGenerator = WebInspector.FlameChart._colorGenerator;

Why do you need local field?

> Source/WebCore/inspector/front-end/FlameChart.js:289
> +        var colorGenerator = this._colorGenerator;

Why do you need this variable?

> Source/WebCore/inspector/front-end/FlameChart.js:348
> +        var colorGenerator = this._colorGenerator;

ditto

[Ilya Tikhonovsky]

Comment on attachment 195729 [details]
Patch

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

>> Source/WebCore/inspector/front-end/FlameChart.js:289
>> +        var colorGenerator = this._colorGenerator;
> 
> Why do you need this variable?

I don't like to access to member field in a cycle which runs huge number of times because usually it reduces performance.

[Ilya Tikhonovsky]

Created attachment 195731 [details]
Patch

[Ilya Tikhonovsky]

Committed r147213: <http://trac.webkit.org/changeset/147213>