Bug 87737 - Web Inspector: draw pie-chart based on memory data received from backend
Summary: Web Inspector: draw pie-chart based on memory data received from backend
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks: 87262
  Show dependency treegraph
 
Reported: 2012-05-29 06:23 PDT by Yury Semikhatsky
Modified: 2012-06-01 01:18 PDT (History)
12 users (show)

See Also:


Attachments
Patch (11.33 KB, patch)
2012-05-29 06:24 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Screenshot (69.79 KB, image/png)
2012-05-29 06:28 PDT, Yury Semikhatsky
no flags Details
Patch (15.89 KB, patch)
2012-05-30 02:13 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (18.29 KB, patch)
2012-05-31 07:01 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (17.70 KB, patch)
2012-05-31 07:11 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (17.62 KB, patch)
2012-05-31 07:12 PDT, Yury Semikhatsky
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2012-05-29 06:23:03 PDT
Profiles view should support drawing pie-charts based on the data received from the inspector memory agent.
Comment 1 Yury Semikhatsky 2012-05-29 06:24:58 PDT
Created attachment 144541 [details]
Patch
Comment 2 Yury Semikhatsky 2012-05-29 06:28:35 PDT
Created attachment 144543 [details]
Screenshot
Comment 3 Yury Semikhatsky 2012-05-29 06:32:12 PDT
All this functionality is still experimental.
Comment 4 Pavel Feldman 2012-05-29 13:00:21 PDT
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?
Comment 5 Yury Semikhatsky 2012-05-30 02:12:02 PDT
(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
Comment 6 Yury Semikhatsky 2012-05-30 02:13:17 PDT
Created attachment 144758 [details]
Patch
Comment 7 Ilya Tikhonovsky 2012-05-31 05:43:43 PDT
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.
Comment 8 Yury Semikhatsky 2012-05-31 07:01:23 PDT
Created attachment 145082 [details]
Patch
Comment 9 Yury Semikhatsky 2012-05-31 07:03:40 PDT
(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.
Comment 10 Ilya Tikhonovsky 2012-05-31 07:10:12 PDT
Comment on attachment 145082 [details]
Patch

please remove Source/WebCore/inspector/front-end/.NativeMemorySnapshotView.js.swo from the patch
Comment 11 Ilya Tikhonovsky 2012-05-31 07:11:10 PDT
Comment on attachment 145082 [details]
Patch

lgtm
Comment 12 Yury Semikhatsky 2012-05-31 07:11:27 PDT
Created attachment 145084 [details]
Patch
Comment 13 Ilya Tikhonovsky 2012-05-31 07:12:27 PDT
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
Comment 14 Yury Semikhatsky 2012-05-31 07:12:47 PDT
Created attachment 145086 [details]
Patch
Comment 15 Yury Semikhatsky 2012-05-31 07:19:14 PDT
(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.
Comment 16 Yury Semikhatsky 2012-06-01 01:18:11 PDT
Committed r119197: <http://trac.webkit.org/changeset/119197>