<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#c7">Comment # 7</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=269943&action=diff" name="attach_269943" title="[PATCH] Proposed Fix">attachment 269943</a> <a href="attachment.cgi?id=269943&action=edit" title="[PATCH] Proposed Fix">[details]</a></span>
[PATCH] Proposed Fix
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=269943&action=review">https://bugs.webkit.org/attachment.cgi?id=269943&action=review</a>
It would be nice to split some of the UI into separate patches so we can examine each with full attention. Right now it's pretty onerous to apply a patch to see how the UI feels.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CircleChart.js:58
> + get centerElement()</span >
A short description of how this SVG is composed (in the constructor?) would help me understand this code. Does the centerElement act like a mask to form the hole?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CircleChart.js:107
> + this.values = new Array(this._values.length).fill(0);</span >
Cute.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CircleChart.js:145
> + "M", x1, y1, // Starting position.</span >
Nice comments. I am worried about the maintainability of constructing SVG manually.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CircleChart.js:156
> + const largeArcFlag = ((a2 - a1) % (Math.PI * 2)) > Math.PI ? 1 : 0;</span >
Please comment what this is computing, for those with less coffee.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.css:28
> + border-bottom: 1px solid rgb(200, 200, 200);</span >
More hsl()
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.css:38
> + color: rgb(200, 200, 200);</span >
ditto
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:34
> + this._category = category;</span >
categoryType?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:39
> + this._detailsElement = this._element.appendChild(document.createElement("div"));</span >
I had to look at the image closely to figure out what all these elements do. Maybe add a reminder?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:70
> + layoutWithDataPoints(dataPoints, lastTime, min, max, timeToX, sizeToY)</span >
I don't understand what the min, max are for. Adding an axis to the name (minX, maxY) would help. If they exist to clip data points outside the selected time interval, then you should do that by slicing dataPoints instead. If they are just for the details blurb, that can be computed in the loop through the points.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:82
> + // Ensure an empty graph is empty.</span >
Why is this not redundant with the preceding bailout?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:88
> + let firstY = sizeToY(dataPoints[0].size);</span >
Please rename these functions to xScale, yScale. This matches standard terminology. For more information about how scales (should) work, d3 has nice docs: <a href="https://github.com/mbostock/d3/wiki/Scales">https://github.com/mbostock/d3/wiki/Scales</a>
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:98
> + // Extend the last data point to the end time.</span >
This could look funny and jitter if the user adjusts the time interval slowly. How often are the resource usage data samples taken? We could draw an extra data point if one exists and clip it.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:100
> + let lastX = Math.floor(timeToX(lastTime));</span >
I would prefer that the scale function takes care of rounding.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:94
> + function timeToX(time) {</span >
See comments above about scales.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:103
> + function sizesToYs(sizes) {</span >
This doesn't appear to be used anywhere.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:161
> + if (lowerIndex > 0)</span >
Should we do the same thing for the upper index?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:165
> + // upperIndex++;</span >
Hmm. What?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:174
> + this._maxSize = Math.max(this._maxSize, memoryTimelineRecord.totalSize);</span >
Nit: _maxCategorySize
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:176
> + if (!this._initializedCategories) {</span >
Nit: _didInitializeCategories
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:179
> + for (let category of memoryTimelineRecord.categories)</span >
Nit: use Array.prototype.map
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.css:84
> + border-right: 1px solid rgb(200, 200, 200);</span >
Nit: hsla().
I'm seeing this particular color a lot in this patch. Should it be a variable?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:192
> + // FIXME: Make MemoryCategoryView resizable.</span >
Nit: add a bug number.
Is this difficult to do? I assume the width caching code is similar to other graphs?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:211
> + get scrollContainerWidth()</span >
Please split all dynamic height-related changes into a separate patch. It's too hard to tell what changes are specific to that.
<span class="quote">> Source/WebKit/mac/ChangeLog:6
> +</span >
Nit: 'Add ENABLE(RESOURCE_USAGE) to feature defines'.</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>