<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#c14">Comment # 14</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:joepeck@webkit.org" title="Joseph Pecoraro <joepeck@webkit.org>"> <span class="fn">Joseph Pecoraro</span></a>
</span></b>
<pre><span class="quote">> > 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.</span >
The reason I passed this in instead of recomputing, is so that this code can update its labels with min/max and immediately handle the special case of max === 0 without having to loop through everything to figure that out.
This is a place that will likely only ever have a single call site, so I'm not worried about parameters.
<span class="quote">> Mostly down to small details now. Please post a new patch (hopefully EWS
> will like it this time).
>
> > Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:101
> > + let lastX = Math.floor(xScale(lastTime));
>
> Is there any reason to floor the value here? Won't it just get clipped by
> the view box?</span >
If lastTime is the current time marker, I want to make sure this doesn't leak past it.
<span class="quote">> > Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:112
> > + const emDash = "\u2014";
>
> I wish we had a lookup table for these somewhere, as I mess them up all the
> time.</span >
I've been waiting for this patch to land before just making emDash a global in Utilities that we can use anywhere.
<span class="quote">> > Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:63
> > + this._updateLegend();
>
> This call seems redundant.
>
> > Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:64
> > + this._chart.clear();
>
> This one too.</span >
These are both required, unless something is guaranteeing a layout.
<span class="quote">> > Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:62
> > + chart2SubtitleElement.textContent = WebInspector.UIString("High Water Mark");
>
> I'm imagining how this will get localized, and dying a little bit. Does
> anyone else have some alternatives?</span >
As you can tell from the original code, I had named this "Max Comparison".
<span class="quote">> > Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:140
> > + this._usageCircleChart.clear();
>
> It is interesting to me that clear() does not already imply needsLayout().</span >
It did previously, but I pulled it out. I did this to reduce the number of unnecessary requestAnimationFrame + immediately cancelAnimationFrame calls that were happening every animation frame (layout).
<span class="quote">> > Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:231
> > + categoryView.layoutWithDataPoints(dataPoints, visibleEndTime, min, max, xScale, yScale);
>
> 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 >
`visibleEndTime` can be the current time marker in the middle of our graph. We don't want to draw past the current time marker.
<span class="quote">> > Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:297
> > + totalElement.textContent = (percent === 100 ? percent : (percent - 0.05).toFixed(1)) + "%";
>
> Why does this lop off .05%? Maybe add a comment.</span >
I did not want to have the text round 99.95-99.99 up to 100 and yet still have the graph showing a non-100% fully drawn in graph. I can add a comment.</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>