[webkit-reviews] review denied: [Bug 87737] Web Inspector: draw pie-chart based on memory data received from backend : [Attachment 144541] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 87737: Web Inspector: draw pie-chart based on memory data received from
backend
https://bugs.webkit.org/show_bug.cgi?id=87737

Attachment 144541: Patch
https://bugs.webkit.org/attachment.cgi?id=144541&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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?