Bug 87737 – Web Inspector: draw pie-chart based on memory data received from backend

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

[Yury Semikhatsky]

Profiles view should support drawing pie-charts based on the data received from the inspector memory agent.

[Yury Semikhatsky]

Created attachment 144541 [details]
Patch

[Yury Semikhatsky]

Created attachment 144543 [details]
Screenshot

[Yury Semikhatsky]

All this functionality is still experimental.

[Pavel Feldman]

Comment on attachment 144541 [details]
Patch

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

> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:56
> +        this._pieChart.updateSize();

Making chart a view would do this for you automatically.

> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:113
> +            profileHeader.memoryBlock = memoryBlock;

Are these public fields on the header? Do they need to be public?

> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:186
> +WebInspector.MemoryBlockViewProperties = function(fillStyle, name, description)

Missing constructor annotation.

> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:188
> +    this.fillStyle = fillStyle;

Are these all public (i.e. visible outside this file?)

> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:193
> +WebInspector._standardMemoryBlocks= null;

Please do not define properties on the global inspector namespace.

> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:195
> +WebInspector.MemoryBlockViewProperties.initialize = function()

should be private

> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:204
> +    addBlock("rgba(240, 240, 250, 0.8)", "ProcessPrivateMemory", "Total");

These should be l18n ready for consistency.

> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:209
> +WebInspector.MemoryBlockViewProperties.forMemoryBlock = function(memoryBlock)

should be private

> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:220
> +WebInspector.NativeMemoryPieChart = function(memorySnapshot)

Missing @constructor declaration. I guess you have not tried compiling it.

> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:257
> +    paint: function()

I think it makes sense to extract a generic PieChart component.

> Source/WebCore/inspector/front-end/heapProfiler.css:264
> +.memory-pie-chart-container {

Is native memory pie chart a part of the heap profiler?

[Yury Semikhatsky]

(In reply to comment #4)
> (From update of attachment 144541 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144541&action=review
> 
> > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:56
> > +        this._pieChart.updateSize();
> 
> Making chart a view would do this for you automatically.
> 
Done.

> > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:113
> > +            profileHeader.memoryBlock = memoryBlock;
> 
> Are these public fields on the header? Do they need to be public?
> 
Yes, they are public on the header, isTemporary is declared on the base class.


> > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:186
> > +WebInspector.MemoryBlockViewProperties = function(fillStyle, name, description)
> 
> Missing constructor annotation.
> 
Fixed.

> > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:188
> > +    this.fillStyle = fillStyle;
> 
> Are these all public (i.e. visible outside this file?)
> 
They are public in this file. And I usually mark such fields as regular
public fields(no _ prefix). This way you can easily see public API of any
class. Another arguable point in marking file-private fields with underscore
is how overridable methods should be named. Do they need to have _ prefix if
they are only overriden in classes in the same file?

Anyways, changed all fields to start with _.

> > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:193
> > +WebInspector._standardMemoryBlocks= null;
> 
> Please do not define properties on the global inspector namespace.
> 
Fixed.

> > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:195
> > +WebInspector.MemoryBlockViewProperties.initialize = function()
> 
> should be private
> 
Fixed.

> > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:204
> > +    addBlock("rgba(240, 240, 250, 0.8)", "ProcessPrivateMemory", "Total");
> 
> These should be l18n ready for consistency.
> 
Fixed.

> > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:209
> > +WebInspector.MemoryBlockViewProperties.forMemoryBlock = function(memoryBlock)
> 
> should be private
> 
Fixed.

> > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:220
> > +WebInspector.NativeMemoryPieChart = function(memorySnapshot)
> 
> Missing @constructor declaration. I guess you have not tried compiling it.
> 
Fixed.

> > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:257
> > +    paint: function()
> 
> I think it makes sense to extract a generic PieChart component.
> 
It is too early at this stage as it is not clear what the API should look like. We'll
be able to extract generic functionality later.

> > Source/WebCore/inspector/front-end/heapProfiler.css:264
> > +.memory-pie-chart-container {
> 
> Is native memory pie chart a part of the heap profiler?
It is hard to say. I will move this into nativeMemoryProfiler.css

[Yury Semikhatsky]

Created attachment 144758 [details]
Patch

[Ilya Tikhonovsky]

Seems to me you have to have the same sets in legend and in pie chart.
Looks like the legend has unnecessary item Used Heap size or you has to split JS Heap size to two (used and free) and change the legend.

[Yury Semikhatsky]

Created attachment 145082 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #7)
> Seems to me you have to have the same sets in legend and in pie chart.
> Looks like the legend has unnecessary item Used Heap size or you has to split JS Heap size to two (used and free) and change the legend.

Done. Removed used JS heap size label.

[Ilya Tikhonovsky]

Comment on attachment 145082 [details]
Patch

please remove Source/WebCore/inspector/front-end/.NativeMemorySnapshotView.js.swo from the patch

[Ilya Tikhonovsky]

Comment on attachment 145082 [details]
Patch

lgtm

[Yury Semikhatsky]

Created attachment 145084 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 145084 [details]
Patch

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

lgtm

> Source/WebCore/ChangeLog:12
> +        * inspector/front-end/.NativeMemorySnapshotView.js.swo: Added.

nit: remove this entry

[Yury Semikhatsky]

Created attachment 145086 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #13)
> (From update of attachment 145084 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145084&action=review
> 
> lgtm
> 
> > Source/WebCore/ChangeLog:12
> > +        * inspector/front-end/.NativeMemorySnapshotView.js.swo: Added.
> 
> nit: remove this entry

Done.

[Yury Semikhatsky]

Committed r119197: <http://trac.webkit.org/changeset/119197>