[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

Brian Burg <bburg at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #270276|review?, commit-queue?      |review-, commit-queue-
              Flags|                            |

--- Comment #13 from Brian Burg <bburg at apple.com> ---
Comment on attachment 270276
  --> https://bugs.webkit.org/attachment.cgi?id=270276
[PATCH] Proposed Fix

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

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?

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

> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:113
> +        this._detailsMaxElement.textContent = WebInspector.UIString("Max: %s").format(Number.isFinite(maxSize) ? Number.bytesToString(maxSize) : emDash);

I don't care about this so much, but Cocoa naming guide warns against abbreviating these. We might have trouble localizing it. What about 'Highest' and 'Lowest'?

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

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:93
> +        let maxCapacity = this._maxSize * 1.05; // add 5% for padding

Nit: '// Add 5% for padding.'

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:141
> +            this._legendElement.textContent = WebInspector.UIString("Max Size: %s").format(Number.bytesToString(this._maxSize));

Same comment as above.

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:42
> +        let chart1Element = overviewElement.appendChild(document.createElement("div"));

Can you please rename chart1, chart2 to describe what they show? Or, at least make it firstChart secondChart.
These should probably be flipped in RTL languages.

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:50
> +        this._usageCircleChart = new WebInspector.CircleChart(120, 0.5);

For constructor parameters like this that are a string of numbers and not called in a tight loop or render function, I highly prefer using the options object approach. Can you change it?

> 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?

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:140
> +        this._usageCircleChart.clear();

It is interesting to me that clear() does not already imply needsLayout().

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:192
> +        // FIXME: Make MemoryCategoryView resizable.

OK, but need a bug number.

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

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:284
> +    _updateMaxComparisonLegend(total)

Please improve parameter's name.

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

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:358
> +        this._maxComparisonMaximumSizeElement = appendLegendRow.call(this, this._maxComparisonLegendElement, "remainder", WebInspector.UIString("Maximum"));

Looking at the screenshot, these labels seem kind of cryptic. I see MB as units, but what is being measured / compared? You definitely will want a tooltip for the legend labels and even the top header.

> Source/WebInspectorUI/UserInterface/Views/StackedLineChart.js:37
> +// - Each path extends all the way down to the bottom, they are layered such

Glorious!

-- 
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/f7f2b296/attachment.html>