<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><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> changed
<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>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #270276 Flags</td>
<td>review?, commit-queue?
</td>
<td>review-, commit-queue-
</td>
</tr></table>
<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#c13">Comment # 13</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>
Mostly down to small details now. Please post a new patch (hopefully EWS will like it this time).
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:101
> + let lastX = Math.floor(xScale(lastTime));</span >
Is there any reason to floor the value here? Won't it just get clipped by the view box?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:112
> + const emDash = "\u2014";</span >
I wish we had a lookup table for these somewhere, as I mess them up all the time.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:113
> + this._detailsMaxElement.textContent = WebInspector.UIString("Max: %s").format(Number.isFinite(maxSize) ? Number.bytesToString(maxSize) : emDash);</span >
I don't care about this so much, but Cocoa naming guide warns against abbreviating these. We might have trouble localizing it. What about 'Highest' and 'Lowest'?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:63
> + this._updateLegend();</span >
This call seems redundant.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:64
> + this._chart.clear();</span >
This one too.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:93
> + let maxCapacity = this._maxSize * 1.05; // add 5% for padding</span >
Nit: '// Add 5% for padding.'
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:141
> + this._legendElement.textContent = WebInspector.UIString("Max Size: %s").format(Number.bytesToString(this._maxSize));</span >
Same comment as above.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:42
> + let chart1Element = overviewElement.appendChild(document.createElement("div"));</span >
Can you please rename chart1, chart2 to describe what they show? Or, at least make it firstChart secondChart.
These should probably be flipped in RTL languages.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:50
> + this._usageCircleChart = new WebInspector.CircleChart(120, 0.5);</span >
For constructor parameters like this that are a string of numbers and not called in a tight loop or render function, I highly prefer using the options object approach. Can you change it?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:62
> + chart2SubtitleElement.textContent = WebInspector.UIString("High Water Mark");</span >
I'm imagining how this will get localized, and dying a little bit. Does anyone else have some alternatives?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:140
> + this._usageCircleChart.clear();</span >
It is interesting to me that clear() does not already imply needsLayout().
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:192
> + // FIXME: Make MemoryCategoryView resizable.</span >
OK, but need a bug number.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:231
> + categoryView.layoutWithDataPoints(dataPoints, visibleEndTime, min, max, xScale, yScale);</span >
As I said above, it should be possible to overdraw the first and last data points and have the bounding box get clipped automatically. So, visibleEndTime might not be necessary to compute.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:284
> + _updateMaxComparisonLegend(total)</span >
Please improve parameter's name.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:297
> + totalElement.textContent = (percent === 100 ? percent : (percent - 0.05).toFixed(1)) + "%";</span >
Why does this lop off .05%? Maybe add a comment.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:358
> + this._maxComparisonMaximumSizeElement = appendLegendRow.call(this, this._maxComparisonLegendElement, "remainder", WebInspector.UIString("Maximum"));</span >
Looking at the screenshot, these labels seem kind of cryptic. I see MB as units, but what is being measured / compared? You definitely will want a tooltip for the legend labels and even the top header.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/StackedLineChart.js:37
> +// - Each path extends all the way down to the bottom, they are layered such</span >
Glorious!</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>