[Webkit-unassigned] [Bug 153516] Web Inspector: High Level Memory Overview Instrument

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=153516

--- Comment #12 from Brian Burg <bburg at apple.com> ---
Comment on attachment 270276
  --> https://bugs.webkit.org/attachment.cgi?id=270276
[PATCH] Proposed Fix

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

> Source/WebCore/ChangeLog:9
> +        Test: inspector/memory/tracking.html

Nit: I usually put the Test: line below the top-level description.

> Source/WebInspectorUI/ChangeLog:40
> +        into a set of 4 massaged categories.

s/massaged/user-facing/

> Source/WebInspectorUI/UserInterface/Main.html:43
> +    <link rel="stylesheet" href="Views/CircleChart.css">

Nit: ordering

> Source/WebInspectorUI/UserInterface/Main.html:525
> +    <script src="Views/MemoryTimelineOverviewGraph.js"></script>

Nit: ordering

> Source/WebInspectorUI/UserInterface/Models/MemoryCategory.js:-32
> -    --z-index-glass-pane-for-drag: 2048;

I hate it when git decides to treat new files as copies due to the copyright block.

> Source/WebInspectorUI/UserInterface/Models/MemoryTimelineRecord.js:36
> +        this._categories = WebInspector.MemoryTimelineRecord.memoryCategoriesFromProtocol(categories);

Nit: I would just call it this._data

> Source/WebInspectorUI/UserInterface/Views/CircleChart.js:26
> +// CircleChart creates a donut/pie chart of colored sections.

Glorious, thank you!

> Source/WebInspectorUI/UserInterface/Views/CircleChart.js:140
> +        this._updateLayout();

Why do these need to forward to private methods?

> Source/WebInspectorUI/UserInterface/Views/LineChart.js:86
> +        this._needsLayout();

Same comment as above.

> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:84
> +        if (!maxSize)

Still not convinced it's going to be much faster to pass in min/max instead of recomputing them. I guess the number of arguments to this function seems a lot to me and easy to mess up.

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:127
> +        this._chart.addPointSet(x, pointSetForRecord(lastRecord));

This code looks much easier to read.

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:155
> +        // if (upperIndex !== records.length)

Did you ever figure this out?

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:28
> +    constructor(timeline, extraArguments)

-- taking a break here --

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160201/e94a74ff/attachment.html>