[Webkit-unassigned] [Bug 153516] Web Inspector: High Level Memory Overview Instrument
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Feb 1 10:42:37 PST 2016
--- Comment #14 from Joseph Pecoraro <joepeck at webkit.org> ---
> > 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.
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.
> 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?
If lastTime is the current time marker, I want to make sure this doesn't leak past it.
> > 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
I've been waiting for this patch to land before just making emDash a global in Utilities that we can use anywhere.
> > 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.
These are both required, unless something is guaranteeing a layout.
> > 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?
As you can tell from the original code, I had named this "Max Comparison".
> > Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:140
> > + this._usageCircleChart.clear();
> It is interesting to me that clear() does not already imply needsLayout().
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).
> > 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.
`visibleEndTime` can be the current time marker in the middle of our graph. We don't want to draw past the current time marker.
> > 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.
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.
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the webkit-unassigned