[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 #7 from Brian Burg <bburg at apple.com> ---
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

It would be nice to split some of the UI into separate patches so we can examine each with full attention. Right now it's pretty onerous to apply a patch to see how the UI feels.

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

> Source/WebInspectorUI/UserInterface/Views/CircleChart.js:107
> +        this.values = new Array(this._values.length).fill(0);

Cute.

> Source/WebInspectorUI/UserInterface/Views/CircleChart.js:145
> +            "M", x1, y1,                    // Starting position.

Nice comments. I am worried about the maintainability of constructing SVG manually.

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

> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.css:28
> +    border-bottom: 1px solid rgb(200, 200, 200);

More hsl()

> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.css:38
> +    color: rgb(200, 200, 200);

ditto

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

categoryType?

> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:39
> +        this._detailsElement = this._element.appendChild(document.createElement("div"));

I had to look at the image closely to figure out what all these elements do. Maybe add a reminder?

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

> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:82
> +        // Ensure an empty graph is empty.

Why is this not redundant with the preceding bailout?

> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:88
> +        let firstY = sizeToY(dataPoints[0].size);

Please rename these functions to xScale, yScale. This matches standard terminology. For more information about how scales (should) work, d3 has nice docs: https://github.com/mbostock/d3/wiki/Scales

> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:98
> +        // Extend the last data point to the end time.

This could look funny and jitter if the user adjusts the time interval slowly. How often are the resource usage data samples taken? We could draw an extra data point if one exists and clip it.

> Source/WebInspectorUI/UserInterface/Views/MemoryCategoryView.js:100
> +        let lastX = Math.floor(timeToX(lastTime));

I would prefer that the scale function takes care of rounding.

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:94
> +        function timeToX(time) {

See comments above about scales.

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:103
> +        function sizesToYs(sizes) {

This doesn't appear to be used anywhere.

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:161
> +        if (lowerIndex > 0)

Should we do the same thing for the upper index?

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

Hmm. What?

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:174
> +        this._maxSize = Math.max(this._maxSize, memoryTimelineRecord.totalSize);

Nit: _maxCategorySize

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:176
> +        if (!this._initializedCategories) {

Nit: _didInitializeCategories

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

Nit: use Array.prototype.map

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

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

Nit: add a bug number.

Is this difficult to do? I assume the width caching code is similar to other graphs?

> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:211
> +    get scrollContainerWidth()

Please split all dynamic height-related changes into a separate patch. It's too hard to tell what changes are specific to that.

> Source/WebKit/mac/ChangeLog:6
> +

Nit: 'Add ENABLE(RESOURCE_USAGE) to feature defines'.

-- 
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/803315fb/attachment-0001.html>