<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: High Level Memory Overview Instrument"
href="https://bugs.webkit.org/show_bug.cgi?id=153516#c12">Comment # 12</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: High Level Memory Overview Instrument"
href="https://bugs.webkit.org/show_bug.cgi?id=153516">bug 153516</a>
from <span class="vcard"><a class="email" href="mailto:bburg@apple.com" title="Brian Burg <bburg@apple.com>"> <span class="fn">Brian Burg</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=270276&action=diff" name="attach_270276" title="[PATCH] Proposed Fix">attachment 270276</a> <a href="attachment.cgi?id=270276&action=edit" title="[PATCH] Proposed Fix">[details]</a></span>
[PATCH] Proposed Fix
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=270276&action=review">https://bugs.webkit.org/attachment.cgi?id=270276&action=review</a>
<span class="quote">> Source/WebCore/ChangeLog:9
> + Test: inspector/memory/tracking.html</span >
Nit: I usually put the Test: line below the top-level description.
<span class="quote">> Source/WebInspectorUI/ChangeLog:40
> + into a set of 4 massaged categories.</span >
s/massaged/user-facing/
<span class="quote">> Source/WebInspectorUI/UserInterface/Main.html:43
> + <link rel="stylesheet" href="Views/CircleChart.css"></span >
Nit: ordering
<span class="quote">> Source/WebInspectorUI/UserInterface/Main.html:525
> + <script src="Views/MemoryTimelineOverviewGraph.js"></script></span >
Nit: ordering
<span class="quote">> Source/WebInspectorUI/UserInterface/Models/MemoryCategory.js:-32
> - --z-index-glass-pane-for-drag: 2048;</span >
I hate it when git decides to treat new files as copies due to the copyright block.
<span class="quote">> Source/WebInspectorUI/UserInterface/Models/MemoryTimelineRecord.js:36
> + this._categories = WebInspector.MemoryTimelineRecord.memoryCategoriesFromProtocol(categories);</span >
Nit: I would just call it this._data
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CircleChart.js:26
> +// CircleChart creates a donut/pie chart of colored sections.</span >
Glorious, thank you!
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CircleChart.js:140
> + this._updateLayout();</span >
Why do these need to forward to private methods?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/LineChart.js:86
> + this._needsLayout();</span >
Same comment as above.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:84
> + if (!maxSize)</span >
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.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:127
> + this._chart.addPointSet(x, pointSetForRecord(lastRecord));</span >
This code looks much easier to read.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:155
> + // if (upperIndex !== records.length)</span >
Did you ever figure this out?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:28
> + constructor(timeline, extraArguments)</span >
-- taking a break here --</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>