[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 #9 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 269943
  --> https://bugs.webkit.org/attachment.cgi?id=269943
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=269943&action=review

>> Source/WebInspectorUI/UserInterface/Views/CircleChart.js:58
>> +    get centerElement()
> 
> 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?

This is just a convenient way to put content in the middle of the donut / circle. It is completely optional. A client of CircleChart may decide to center some text inside the Circle. By getting chart.centerElement it gets a div already positioned correctly in the center, whatever the radius dimensions were for the circle chart.

>> Source/WebInspectorUI/UserInterface/Views/CircleChart.js:156
>> +        const largeArcFlag = ((a2 - a1) % (Math.PI * 2)) > Math.PI ? 1 : 0;
> 
> Please comment what this is computing, for those with less coffee.

This code is all copied from RenderingFrame's circle chart.

>> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:34
>> +        this._category = category;
> 
> categoryType?

This is why I preferred the name MemoryCategory over MemoryCategoryData. If I change the name of MemoryCategory to MemoryCategoryData these would all change to categoryData.

>> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:70
>> +    layoutWithDataPoints(dataPoints, lastTime, min, max, timeToX, sizeToY)
> 
> 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.

These are the min and max values in the data points, that don't need to be recalculated.

>> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:82
>> +        // Ensure an empty graph is empty.
> 
> Why is this not redundant with the preceding bailout?

A graph with min and max 0, and a data point of [0] would still draw something. This ensures nothing draws.

>> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:103
>> +        function sizesToYs(sizes) {
> 
> This doesn't appear to be used anywhere.

Yep! Good catch. It was replaced by the one below.

>> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:165
>> +        //     upperIndex++;
> 
> Hmm. What?

I was seeing an issue while current time was moving forward. If there was memory data that came in slightly before current time reached that point, you would see the memory graph draw past current time. This code could be removed, but I'd like to see it tweaked to be better.

>> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:179
>> +            for (let category of memoryTimelineRecord.categories)
> 
> Nit: use Array.prototype.map

I don't want to modify the original list of categories.

>> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.css:84
>> +    border-right: 1px solid rgb(200, 200, 200);
> 
> Nit: hsla().
> 
> I'm seeing this particular color a lot in this patch. Should it be a variable?

Well you reminded me I need to make window-inactive colors! A variable may be nice here.

>> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:46
>> +        let chart1Element = overviewElement.appendChild(document.createElement("div"));
> 
> UI nit: it's not clear from the static screenshot where the left chart gets its data. Is it from one single instant? If so, add a subtitle.

You are correct, it is from the last data point.

-- 
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/20160128/0bdaf759/attachment.html>