[Webkit-unassigned] [Bug 153516] Web Inspector: High Level Memory Overview Instrument

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=153516

--- 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
> time.

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...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160201/93abd474/attachment-0001.html>