Bug 153516 – Web Inspector: High Level Memory Overview Instrument

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
High Level Memory Overview Instrument.

It can be useful to get a breakdown of where a Page's memory is. Images, Layers, JavaScript, C++ (bmalloc, Lib C Malloc / system malloc), Other. WebCore already has this data from the ResourceUsageThread / ResourceUsageOverlay. Display this information in the Inspector as well, if the backend supports gathering the data.

[Radar WebKit Bug Importer]

<rdar://problem/24356378>

[Joseph Pecoraro]

Created attachment 269934 [details]
[IMAGE] First Draft

Includes:
  - stacked line chart showing total memory use in the timeline
  - per-category line chart in the detailed Timeline View
  - category breakdown and max usage comparison charts

[Joseph Pecoraro]

Created attachment 269943 [details]
[PATCH] Proposed Fix

Missing in this patch:
  - MemoryTimelineView is not responsive, won't work well when Inspector is narrow / short (docked)
  - no Memory Timeline View icon yet
  - no UI to pick instruments, so this instrument is not reachable yet without modifying the code

[Blaze Burg]

Comment on attachment 269943 [details]
[PATCH] Proposed Fix

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

Backend parts look really clean.

> LayoutTests/ChangeLog:11
> +        Basic test for the new domain tracking commands ane events.

typo: 'and'

> LayoutTests/inspector/memory/tracking.html:26
> +                let types = categories.reduce((list, x) => { list.push(x.type); return list; }, []).sort();

This should be Array.prototype.map.

> Source/JavaScriptCore/ChangeLog:8
> +

Message: 'Add ENABLE(RESOURCE_USAGE). This is already used in WebCore.'

> Source/JavaScriptCore/inspector/protocol/Memory.json:8
> +            "id": "Event",

I can't figure out this name. Why? I would have said 'ResourceUsageData' or something.

> Source/JavaScriptCore/inspector/protocol/Memory.json:16
> +            "id": "Category",

I was expecting 'Category' to be the name of the enum. Please rename to CategoryData.

> Source/JavaScriptCore/inspector/protocol/Memory.json:39
> +                { "name": "timestamp", "type": "number" }

One parameter per line, please. That is how most inspector protocol specifications are formatted.

> Source/JavaScriptCore/inspector/protocol/Memory.json:44
> +            "description": "While tracking is enabled the backend will periodically update the frontend with event data.",

Nit: 'after tracking has started' (there's no 'enabled command)

> Source/JavaScriptCore/inspector/protocol/Memory.json:46
> +                { "name": "event", "$ref": "Event" }

Ditto.

> Source/JavaScriptCore/inspector/protocol/Memory.json:51
> +            "description": "When tracking is complete."

Please unify the tense of the tracking event descriptions.

"Memory usage tracking [has] started/stopped."

> Source/JavaScriptCore/inspector/scripts/codegen/generator.py:41
> +_ALWAYS_UPPERCASED_ENUM_VALUE_SUBSTRINGS = set(['API', 'CSS', 'DOM', 'HTML', 'JIT', 'XHR', 'XML'])

Did you rebaseline the generator tests?

> Source/WebCore/ChangeLog:8
> +

'Add a new agent that gathers data from the ResourceUsageThread...'

> Source/WebCore/inspector/InspectorMemoryAgent.cpp:82
> +void InspectorMemoryAgent::sample(const ResourceUsageData& data)

I would rename it to 'collectSample'. It's unclear otherwise that sample is the verb or noun.

> Source/WebCore/inspector/InspectorMemoryAgent.h:66
> +#endif // !defined(InspectorMemoryAgent_h)

Newline at EOF?

[Blaze Burg]

Comment on attachment 269943 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/ChangeLog:8
> +

Needs summary.

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:66
> +        // FIXME: UI To pick and choose Instruments.

What?

> Source/WebInspectorUI/UserInterface/Models/MemoryCategory.js:26
> +WebInspector.MemoryCategory = class MemoryCategory extends WebInspector.Object

Again, this is an off-putting name for the thing that holds pieces of data and can have lots of instances. At the least, add a -Data suffix to make it clearer.

> Source/WebInspectorUI/UserInterface/Models/MemoryTimelineRecord.js:86
> +    get totalSize() { return this._totalSize; }

Please add a line here for // TimelineRecord API

> Source/WebInspectorUI/UserInterface/Protocol/MemoryObserver.js:2
> + * Copyright (C) 2015 Apple Inc. All rights reserved.

oops

> Source/WebInspectorUI/UserInterface/Views/CircleChart.css:31
> +    fill: rgba(0, 0, 0, 0.02);

hsla()?

[Joseph Pecoraro]

Comment on attachment 269943 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/inspector/memory/tracking.html:26
>> +                let types = categories.reduce((list, x) => { list.push(x.type); return list; }, []).sort();
> 
> This should be Array.prototype.map.

I suppose I could, but I didn't want to modify the list the incoming list. Side-effects make it hard to debug in the future.

>> Source/JavaScriptCore/inspector/protocol/Memory.json:8
>> +            "id": "Event",
> 
> I can't figure out this name. Why? I would have said 'ResourceUsageData' or something.

That was just going to be my trend:

    ScriptProfiler.startTracking => ScriptProfiler.Event in trackingUpdate
    Memory.startTracking => Memory.Event in trackingUpdate
    Heap.startTracking => Heap.Event in trackingUpdate

>> Source/JavaScriptCore/inspector/protocol/Memory.json:39
>> +                { "name": "timestamp", "type": "number" }
> 
> One parameter per line, please. That is how most inspector protocol specifications are formatted.

?

>> Source/JavaScriptCore/inspector/scripts/codegen/generator.py:41
>> +_ALWAYS_UPPERCASED_ENUM_VALUE_SUBSTRINGS = set(['API', 'CSS', 'DOM', 'HTML', 'JIT', 'XHR', 'XML'])
> 
> Did you rebaseline the generator tests?

I didn't but this string would not have existed.

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:66
>> +        // FIXME: UI To pick and choose Instruments.
> 
> What?

The new Timelines UI will allow adding / removing / changing instruments. I'll just remove these lines in my next iteration of the patch, but this is where you have to manually add the instrument to test it until the new picker/configuration UI becomes available.

[Blaze Burg]

Comment on attachment 269943 [details]
[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'.

[Blaze Burg]

Comment on attachment 269943 [details]
[PATCH] Proposed Fix

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

Not too much work left before this is ready to go. Please post a patch that applies cleanly so I can try it out! :) Also, please split the variable-height overview graph thing.

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:113
> +        function ysForRecord(record) {

Weird name. Maybe 'scaledPointSetForRecord'. Although I would really prefer that the chart itself be passed a scale and take responsibility for applying it as necessary. I can't think of any reason why the scale would vary by category, so just two scales is enough.

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:45
> +

These variable names are funny =) but you should put in what the chart contains.

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

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:50
> +        chart1SubtitleElement.textContent = WebInspector.UIString("Breakdown");

Maybe "Memory Usage by Category" ?

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

UI nit: on the right chart, I would use a different color than that also used for Page. This chart also lacks a subtitle documenting the data range incorporated into the chart.

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:65
> +        chart2SubtitleElement.textContent = WebInspector.UIString("High Water Mark");

This might be a big jargony for typical web developer who is just learning about memory usage. Can you add some explainer text in the tooltip?

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:188
> +        this._maxComparisonCircleChart.values = [lastRecord.totalSize, this._maxSize - lastRecord.totalSize];

Ah, I guess that answers my question. It seems to be over the entire recording, not the selected range.

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:264
> +            totalElement.appendChild(document.createElement("span")); // firstChild

You can drop the comment.

> Source/WebInspectorUI/UserInterface/Views/StackedLineChart.js:68
> +    initializeSections(list)

Nit: s/list/classNames/ or similar

> Source/WebInspectorUI/UserInterface/Views/StackedLineChart.js:122
> +

Please add a comment describing the composition strategy. There are lots of ways to achieve stacked line graphs, some worse than others.

[Joseph Pecoraro]

Comment on attachment 269943 [details]
[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.

[Joseph Pecoraro]

Created attachment 270276 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 270277 [details]
[IMAGE] Second Draft

[Blaze Burg]

Comment on attachment 270276 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/ChangeLog:9
> +        Test: inspector/memory/tracking.html

Nit: I usually put the Test: line below the top-level description.

> Source/WebInspectorUI/ChangeLog:40
> +        into a set of 4 massaged categories.

s/massaged/user-facing/

> Source/WebInspectorUI/UserInterface/Main.html:43
> +    <link rel="stylesheet" href="Views/CircleChart.css">

Nit: ordering

> Source/WebInspectorUI/UserInterface/Main.html:525
> +    <script src="Views/MemoryTimelineOverviewGraph.js"></script>

Nit: ordering

> Source/WebInspectorUI/UserInterface/Models/MemoryCategory.js:-32
> -    --z-index-glass-pane-for-drag: 2048;

I hate it when git decides to treat new files as copies due to the copyright block.

> Source/WebInspectorUI/UserInterface/Models/MemoryTimelineRecord.js:36
> +        this._categories = WebInspector.MemoryTimelineRecord.memoryCategoriesFromProtocol(categories);

Nit: I would just call it this._data

> Source/WebInspectorUI/UserInterface/Views/CircleChart.js:26
> +// CircleChart creates a donut/pie chart of colored sections.

Glorious, thank you!

> Source/WebInspectorUI/UserInterface/Views/CircleChart.js:140
> +        this._updateLayout();

Why do these need to forward to private methods?

> Source/WebInspectorUI/UserInterface/Views/LineChart.js:86
> +        this._needsLayout();

Same comment as above.

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

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:127
> +        this._chart.addPointSet(x, pointSetForRecord(lastRecord));

This code looks much easier to read.

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:155
> +        // if (upperIndex !== records.length)

Did you ever figure this out?

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:28
> +    constructor(timeline, extraArguments)

-- taking a break here --

[Blaze Burg]

Comment on attachment 270276 [details]
[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!

[Joseph Pecoraro]

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

[Joseph Pecoraro]

Created attachment 270421 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 270422 [details]
[PATCH] For Bots (no localized strings)

[WebKit Commit Bot]

Attachment 270421 [details] did not pass style-queue:


ERROR: LayoutTests/platform/gtk/TestExpectations:669:  expecting "[", "#", or end of line instead of "PROGRESS:"  [test/expectations] [5]
ERROR: LayoutTests/platform/gtk/TestExpectations:669:  Path does not exist.  [test/expectations] [5]
WARNING: Not running on native Windows.
Total errors found: 2 in 50 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Attachment 270422 [details] did not pass style-queue:


ERROR: LayoutTests/platform/gtk/TestExpectations:669:  expecting "[", "#", or end of line instead of "PROGRESS:"  [test/expectations] [5]
ERROR: LayoutTests/platform/gtk/TestExpectations:669:  Path does not exist.  [test/expectations] [5]
WARNING: Not running on native Windows.
Total errors found: 2 in 50 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 270421 [details]
[PATCH] Proposed Fix

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

> LayoutTests/platform/gtk/TestExpectations:669
> +webkit.org/b/153602 fast/table/003.html [ Failure ]
> +webkit.org/b/153602 fast/text/emoji.html [ Failure ]
> +webkit.org/b/153602 tables/mozilla_expected_failures/bugs/bug89315.html [ Failure ]
> +>>>>>>> PROGRESS: High Level Memory Overview

Oops. I'll remove these lines, they were a merge conflict.

[Blaze Burg]

Comment on attachment 270421 [details]
[PATCH] Proposed Fix

r=me, let's try this!

[Joseph Pecoraro]

<http://trac.webkit.org/changeset/195999>

[Csaba Osztrogonác]

(In reply to comment #21)
> <http://trac.webkit.org/changeset/195999>

It broke the Apple Mac cmake build, see https://build.webkit.org/builders/Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/2144 for details.

[Joseph Pecoraro]

(In reply to comment #22)
> (In reply to comment #21)
> > <http://trac.webkit.org/changeset/195999>
> 
> It broke the Apple Mac cmake build, see
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/2144 for details.

Thanks for the heads up.

It looks like "Inspector::MemoryBackendDispatcherHandler" doesn't exist, which means ENABLE_RESOURCE_USAGE probably didn't get set on the CMake build. Now I just have to find out how to enable it.

[Joseph Pecoraro]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > <http://trac.webkit.org/changeset/195999>
> > 
> > It broke the Apple Mac cmake build, see
> > https://build.webkit.org/builders/
> > Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/2144 for details.
> 
> Thanks for the heads up.
> 
> It looks like "Inspector::MemoryBackendDispatcherHandler" doesn't exist,
> which means ENABLE_RESOURCE_USAGE probably didn't get set on the CMake
> build. Now I just have to find out how to enable it.

Addressed in:
<https://trac.webkit.org/changeset/196020>

There is now a separate build issue, I'll ping people.