Bug 195202 – Web Inspector: CPU Usage Timeline - Statistics and Sources sections

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

CPU Usage Timeline - Statistics and Sources sections

Analyze all of the Script / Layout / Rendering records in the selected time range and show some useful stats such as:

  - types and counts for timers / events / observers / script entries
  - counts of handlers / timer installers based on source location

This gives a very good high level overview of how script evaluted, which can be further investigated.

[Joseph Pecoraro]

Created attachment 363298 [details]
[IMAGE] Dark Mode - Filter

[Joseph Pecoraro]

Created attachment 363299 [details]
[IMAGE] Dark Mode - No Filter

[Joseph Pecoraro]

Created attachment 363300 [details]
[IMAGE] Dark Mode - Empty

[Joseph Pecoraro]

Created attachment 363301 [details]
[IMAGE] Light Mode - Filter

[Joseph Pecoraro]

Created attachment 363302 [details]
[IMAGE] Light Mode - No Filter

[Joseph Pecoraro]

Created attachment 363303 [details]
[IMAGE] Light Mode - Empty

[Joseph Pecoraro]

Created attachment 363304 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 363404 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 363405 [details]
[IMAGE] Filter Clear Button

[Nikita Vasilyev]

Created attachment 363432 [details]
[Image] Equal-sized columns

I don't think it's an effective use of space to have both columns take 50% of the width. The left column should be more narrow. Perhaps it makes sense for the columns to be content-dependent.

[Joseph Pecoraro]

(In reply to Nikita Vasilyev from comment #10)
> Created attachment 363432 [details]
> [Image] Equal-sized columns
> 
> I don't think it's an effective use of space to have both columns take 50%
> of the width. The left column should be more narrow. Perhaps it makes sense
> for the columns to be content-dependent.

Yes, I have considered splitting it up more as one section (maybe even dropping the title) to make better user of vertical and horizontal space.

>                                                   Sources
>                                                   -------
>  
>  Script Entries: ###        Timers: ### (setTimeout)                          Timers: ### asdf (ad.js:#:#)
>         Layouts:   #                 ## (requestAnimationFrame)                        ## draw (draw.js:#:#)
>          Paints:  ##        Events:   # (scroll)                                        # schedule (script.js:#:#)
>   Style Recalcs:   #                  # (load)                        Event Handlers:  ## (ad.js:#:#)
>    Frames Drawn:  ##                  # (message)                                       # (event.js:#:#)
>                          Observers:   # (MutationObserver)                              # (script.js:#:#)
>                                                                                         # (script.js:#:#)
>                                                                    Observer Handlers:   # (script.js:#:#)

[Nikita Vasilyev]

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

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

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:321
> +.timeline-view.cpu > .content > .overview > .chart > .container.stats {

I suggest to allow copying text here by adding:

    -webkit-user-select: text;

I can see myself:
- copying and sharing this data.
- copying method names to find them in my IDE.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:333
> +        let statisticsContainerElement = createChartContainer(bottomOverviewElement, WI.UIString("Statistics"), "");

This section is pretty self-explanatory. I'd prefer to omit the subtitle here.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:350
> +        let sourcesContainerElement = createChartContainer(bottomOverviewElement, WI.UIString("Sources"), "");

Ditto.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:358
> +            headerCell.textContent = WI.UIString("Filter:");

I don't see the value of duplicating active filters here — they're visible on the left.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:783
> +            let span = document.createElement("span");
> +            span.className = "show-more";
> +            span.textContent = ellipsis;
> +            span.addEventListener("click", (event) => {
> +                expandAllSections();
> +            });

I'd style this similarly to the buttons that expand the prototype chain (`.item.object-tree-property.prototype-property`) and use the text "more..." to increase the clickable area.

Also, please add:
    span.role = "button"

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:833
> +    _layoutSourcesSection(statistics)

Long method names don't wrap and can add undesired horizontal scrolling for the content view. I suggest wrapping method names.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:857
> +                return WI.UIString("Unknown Location");

I'd style "Unknown Location" text differently from method names. Perhaps:

    color: var(--text-color-secondary);
    font-style: italic;

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:358
>> +            headerCell.textContent = WI.UIString("Filter:");
> 
> I don't see the value of duplicating active filters here — they're visible on the left.

Two reasons:

  1. The filters stay active as you change the selected time range. So you may enable the `setTimeout` filter, then select a range that doesn't have `setTimeout` and want to know why no results are happening.
  2. Having the filters above the thing they are filtering allows you to remove/clear the filters quickly/easily.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:783
>> +            });
> 
> I'd style this similarly to the buttons that expand the prototype chain (`.item.object-tree-property.prototype-property`) and use the text "more..." to increase the clickable area.
> 
> Also, please add:
>     span.role = "button"

Good suggestion on the role. I kept this as compact much as possible to reduce vertical space waste / clutter. I agree 3 dots is hard to notice as a clickable region but I think any developer curious what has been left out would click it. I'll look into adding more text.

[Devin Rousso]

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

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

Looks good!  I'd like to see another iteration (preferably using `needsLayout`) before I finalize my review.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:121
> +Object.defineProperty(Map.prototype, "getOrInitialize",

Is this something we care about testing?  It seems pretty trivial...

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:321
>> +.timeline-view.cpu > .content > .overview > .chart > .container.stats {
> 
> I suggest to allow copying text here by adding:
> 
>     -webkit-user-select: text;
> 
> I can see myself:
> - copying and sharing this data.
> - copying method names to find them in my IDE.

I agree with both, but I think I'd be a bit more restrictive as to what we allow to be copied.  As an example, the filter "bubbles" don't (and probably shouldn't) need to be copied.  I think we should limit it to the Sources counts and names/locations.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:352
> +    color: white;

Can we use a variable for this instead?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:354
> +    background-color: darkgray;

Ditto (>352).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:355
> +    background-color: darkgray;
> +    border-radius: 50%;

NIT: I normally put `background` and `border` properties after "content" properties, like `font-size` and `line-height` and `text-align`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:364
> +    border-radius: 3px;
> +    border: 1px solid transparent;

NIT: I normally put `border` after `background` and `border-radius` after `border`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:366
> +    background-color: hsl(0, 0%, 85%);

Ditto (>352).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:371
> +.timeline-view.cpu > .content > .overview > .chart > .container.stats > table .filter-clear:hover,
> +.timeline-view.cpu > .content > .overview > .chart > .container.stats > table .filter:hover {

NIT: `:matches(.filter-clear, .filter):hover`?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:377
> +    color: var(--selected-foreground-color);

NIT: I normally put `color` before `background`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:389
> +    .timeline-view.cpu > .content > .overview > .chart > .container.stats > table .filter-clear {

Merge this with the rule above it.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:390
> +        background-color: gray;

Ditto (>352).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:399
> +        background-color: hsl(0, 0%, 33%);

Ditto (>352).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:38
> +        this._sectionLimit = 5;

The `5` would be great as a `static get`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:126
> +        this._sectionLimit = 5;

Ditto (>38).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:-291
> -            this.updateLayout();

NIT: we should be using `needsLayout` instead, so we don't run the risk of double drawing.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:300
> +            this.updateLayout(WI.CPUTimelineView.LayoutReason.Internal);

This feels a little like cheating :/  but I can't really find a reason not to like it at the same time :P

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:324
> +        this._sourcesFilters = {

NIT: I think we usually don't make these plural, as it read's a little weird to say `this._sourcesFilters.timer` vs `this._sourceFilter.timer`.  Please make it singular.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:327
> +            timer: new Set,
> +            event: new Set,
> +            observer: new Set,

Rather than having these keys be something you have to remember, we could leverage computed keys and a static object.
```
    this._sourcesFilter = {};
    for (let type in Object.values(WI.CPUTimelineView.Source))
        this._sourceFilter[type] = new Set;

    WI.CPUTimelineView.Source = {
        Timer: "timer",
        Event: "event",
        Observer: "observer",
    };
```

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:366
> +            filterClearElement.textContent = "\u00D7";

Can you make this into a variable (or put it in Utilities.js) so we know what it is without having to look it up?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:369
> +                this.updateLayout(WI.CPUTimelineView.LayoutReason.Internal);

Ditto (>291).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:372
> +        }
> +        {

Style: there should be a newline between these.  Reading it as it is right now, I think it's like an array.  e.g.
```
    [
        {
            ...
        },
        {
            ...
        },
    ]
```

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:379
> +        }
> +        {

Ditto (>371).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:386
> +        }
> +        {

Ditto (>371).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:405
> +            this._sectionLimit = 5;

Ditto (>38).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:745
> +        let updateFilterAndSections = (element, type, name) => {

Why not inline this?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:757
> +            span.className = "filter";

NIT: `classList.add`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:759
> +            span.addEventListener("mouseup", (event) => {

Why not use `"click"`?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:764
> +            if (this._sourcesFilters[type].has(name))
> +                span.classList.add("active");

`span.classList.toggle("active", this._sourcesFilters[type].has(name))`

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:769
> +        let sectionLimit = this._sectionLimit;

Why did you save this to a local?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:771
> +        let expandAllSections = () => {

This function could be inlined, as it's only used once.  Alternatively, you could make this into a member function and share it between `_layoutStatisticsSection` and `_layoutSourcesSection`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:779
> +            span.className = "show-more";

Ditto (>757).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:788
> +            let i = 0;

Rather than keep a separate counter, you could just use `this._statisticRows.length`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:789
> +            let sorted = new Map([...statistics.timerTypes.entries()].sort((a, b) => b[1] - a[1]));

`Array.from(statistic.timerTypes)` is simpler and should be more efficient.
I'd pull the sort function out into it's own function so that it can be reused (and commented/explained).  It took me a little bit of time to track down what `typerTypes` is and how it's structured, so some comment/variable explaining what `a[1]` and `b[1]` actually _mean_ would be nice.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:791
> +                let heading = !i ? WI.UIString("Timers:") : "";

NIT: I personally find that `!i` is very hard to read, so I prefer using `i ===0` (as well as the inverted `i > 0`) in these cases as it's a bit clearer.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:803
> +            let i = 0;

Ditto (>788).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:804
> +            let sorted = new Map([...statistics.eventTypes.entries()].sort((a, b) => b[1] - a[1]));

Ditto (>789).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:806
> +                let heading = !i ? WI.UIString("Events:") : "";

Ditto (>791).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:818
> +            let i = 0;

Ditto (>788).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:819
> +            let sorted = new Map([...statistics.observerTypes.entries()].sort((a, b) => b[1] - a[1]));

Ditto (>789).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:821
> +                let heading = !i ? WI.UIString("Observers:") : "";

Ditto (>791).

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:833
>> +    _layoutSourcesSection(statistics)
> 
> Long method names don't wrap and can add undesired horizontal scrolling for the content view. I suggest wrapping method names.

We are planning on making this into a CSS grid, which will allow us to use `text-overflow: ellipsis;`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:837
> +        const unknownLocationKey = "unknown";

`Symbol("unknown")`?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:839
> +        function firstNonNativeCallFrame(callFrames) {

This looks like it does the same thing as `WI.TimelineRecord.prototype.initiatorCallFrame`.  Can you use that instead?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:852
> +            return sourceCodeLocation.sourceCode.url + ":" + sourceCodeLocation.lineNumber + ":" + sourceCodeLocation.columnNumber;

`sourceCodeLocation.originalLocationString(WI.SourceCodeLocation.ColumnStyle.Shown, WI.SourceCodeLocation.NameStyle.Short)`?

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:857
>> +                return WI.UIString("Unknown Location");
> 
> I'd style "Unknown Location" text differently from method names. Perhaps:
> 
>     color: var(--text-color-secondary);
>     font-style: italic;

+1

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:872
> +        let hasFilters = (timerFilters.size || eventFilters.size || observerFilters.size);

Style: unnecessary parenthesis.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:876
> +            sectionLimit = 10;

Ditto (>38).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:878
> +        let expandAllSections = () => {

Ditto (>771).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:886
> +            span.className = "show-more";

Ditto (>757).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:913
> +                let functionName = callFrame ? callFrame.functionName : "";
> +                let key = keyForSourceCodeLocation(sourceCodeLocation);

NIT: since `key` is used before `functionName` in `getOrInitialize`, I'd put `key` above `functionName`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:918
> +                    let count = repeatingEntry ? repeatingEntry.count : 1;

NIT: you could inline this.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:919
> +                    entry.count += count;

Just making sure I understand; is it guaranteed that a repeating timer will only appear in `repeatingTimers` once (e.g. I'm worried about double counting because of the `+=`).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:920
> +                    entry.repeating = record.details.repeating;

Should we only be setting this in the `true` case?  IIUC, it's wouldn't be possible for a repeating and non-repeating timer to both be fired from the same location, so this would be "consistent" between iterations.  Should we be asserting as such?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:923
> +                    entry.count += 1;

`++entry.count;`

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:926
> +            // Aggregate repeating timers where we did did not see the installation in the selected time range.

Typo: "did did".

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:930
> +                    if (!seenTimers.has(timerId)) {

NIT: invert and make this an early-continue?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:932
> +                        // FIXME: We should have a map of all repeating timer installations in the whole recording
> +                        // so that we can provide a function name for these repeating timers lacking an installation point.

Please file a bug for this so it doesn't get forgotten.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:950
> +                entry.count += 1;

Ditto (>923).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:961
> +                entry.count += 1;

Ditto (>923).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:968
> +            let i = 0;

Ditto (>788).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:969
> +            let sorted = new Map([...timerMap.entries()].sort((a, b) => b[1].count - a[1].count));

Ditto (>789).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:971
> +                let number = entry.repeating ? "~" + entry.count : entry.count;

Should this be localized?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:975
> +                let row;

Style: this should be initialized to some value (e.g. `null`).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:976
> +                if (!i) {

Ditto (>791).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:984
> +                    row.querySelector(".label").append(` ${enDash} ${entry.functionName}`);

Rather than querying for an element, you could just add this at the end of `label` before you pass it around.
```
    if (entry.functionName)
        label += ` ${enDash} ${entry.functionName}`;
```

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:994
> +            let i = 0;

Ditto (>788).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:995
> +            let sorted = new Map([...eventHandlerMap.entries()].sort((a, b) => b[1].count - a[1].count));

Ditto (>789).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1000
> +                if (!i) {

Ditto (>791).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1014
> +            let i = 0;

Ditto (>788).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1015
> +            let sorted = new Map([...observerCallbackMap.entries()].sort((a, b) => b[1].count - a[1].count));

Ditto (>789).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1020
> +                if (!i) {

Ditto (>791).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1107
> +        function incrementTypeCount(map, key) {

NIT: I'd rename this as just `incrementCount`, as there's nothing particularly "specific" to "Type" in the body.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1112
> +            if (entry)
> +                map.set(key, entry + 1);
> +            else
> +                map.set(key, 1);

You could leverage the new utility you added.
```
    let existing = map.getOrInitialize(key, 0);
    map.set(key, existing + 1);
```

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1129
> +            // Return true for event types that define script entry/exits.

Grammar: s/entry/entries

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1130
> +            // Return false for events with no time ranges or would be contained in other events.

Grammar: s/would be/if they are

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1157
> +                    var entry = repeatingTimers.get(record.details);

Style: add `{...}` around the case body and make this `let`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1159
> +                        entry.count++;

Ditto (>923).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1161
> +                    var entry = repeatingTimers.get(record.details);
> +                    if (entry)
> +                        entry.count++;
> +                    else
> +                        repeatingTimers.set(record.details, {record, count: 1});

Ditto (>1109).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1174
> +                // These event types have no time range, or are contained by the others.

Grammar: s/others/other events

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1321
> +        this._sourcesFilterRow.hidden = true;
> +        this._sourcesFilterLabelElement.removeChildren();
> +
> +        this._timerInstallationsRow.hidden = false;
> +        this._eventHandlersRow.hidden = false;
> +        this._observerHandlersRow.hidden = false;

Ditto (>291).

Rather than change the DOM's state here, we could do that in `_updateSourcesFilters` (or `layout`).  Then this function could also call `needsLayout` for you.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1340
> +    _updateSourcesFilters()

Ditto (>291).

The logic in this function should happen inside a `layout`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1353
> +            span.className = "filter active";

Ditto (>757).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1355
> +            span.addEventListener("mouseup", (event) => {

Ditto (>759).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1357
> +                this.updateLayout(WI.CPUTimelineView.LayoutReason.Internal);

Ditto (>291).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1370
> +        for (let name of timerFilters)
> +            this._sourcesFilterLabelElement.appendChild(createActiveFilterElement("timer", name));
> +        for (let name of eventFilters)
> +            this._sourcesFilterLabelElement.appendChild(createActiveFilterElement("event", name));
> +        for (let name of observerFilters)
> +            this._sourcesFilterLabelElement.appendChild(createActiveFilterElement("observer", name));

Does this do any wrapping or allow scrolling?  If a lot of filters are selected, does that mean it extends past the edge of the window?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1389
> +        return [row, headerCell, numberCell, labelCell];

Rather than return an array, why not use an object?  That would allow you to avoid having to create many of the closures (and extra destructuring) when calling `_createTableRow`?

    return {row, headerCell, numberCell, labelCell};

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1392
> +    _insertTableRow(table, rowList, heading, numberValue, labelValue, followingRow = null)

I'd refactor this to have all required properties be listed and all non-required properties be in an object.  That way, for the cases where `heading` or `numberValue` are `""`, we don't need to write it.

    _insertTableRow(table, rows, {headerValue, numberValue, labelValue, followingRow} = {})

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1417
> +        for (let row of this._statisticsRows)
> +            row.remove();

Is this the equivalent of `this._statisticsTable.removeChildren`?

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:121
>> +Object.defineProperty(Map.prototype, "getOrInitialize",
> 
> Is this something we care about testing?  It seems pretty trivial...

Yeah, I thought about it and passed just because it was so simple. But I added one.

>>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:321
>>> +.timeline-view.cpu > .content > .overview > .chart > .container.stats {
>> 
>> I suggest to allow copying text here by adding:
>> 
>>     -webkit-user-select: text;
>> 
>> I can see myself:
>> - copying and sharing this data.
>> - copying method names to find them in my IDE.
> 
> I agree with both, but I think I'd be a bit more restrictive as to what we allow to be copied.  As an example, the filter "bubbles" don't (and probably shouldn't) need to be copied.  I think we should limit it to the Sources counts and names/locations.

Text selection works great. The filter bubbles select great as well since they are just text.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:38
>> +        this._sectionLimit = 5;
> 
> The `5` would be great as a `static get`.

Done.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:327
>> +            observer: new Set,
> 
> Rather than having these keys be something you have to remember, we could leverage computed keys and a static object.
> ```
>     this._sourcesFilter = {};
>     for (let type in Object.values(WI.CPUTimelineView.Source))
>         this._sourceFilter[type] = new Set;
> 
>     WI.CPUTimelineView.Source = {
>         Timer: "timer",
>         Event: "event",
>         Observer: "observer",
>     };
> ```

This seems rather overkill. Is there any advantage?

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:366
>> +            filterClearElement.textContent = "\u00D7";
> 
> Can you make this into a variable (or put it in Utilities.js) so we know what it is without having to look it up?

Heh, it already was: `multiplicationSign`. I missed it because of a case sensitive search.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:757
>> +            span.className = "filter";
> 
> NIT: `classList.add`.

className is faster for a newly created element with a known list. I believe that is our preferred style, at least that is what I have always seen and done.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:759
>> +            span.addEventListener("mouseup", (event) => {
> 
> Why not use `"click"`?

We do "mouseup" instead of "click" because during a recording the elements change. So the one you mousedown'd on is different from the one you mouseup'd on and click doesn't work, but mouseup does.

We do have a flicker issue with mouse hovering these during an active recording already.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:769
>> +        let sectionLimit = this._sectionLimit;
> 
> Why did you save this to a local?

This matches the style of the other section generation, which doubled the sectionLimit when there was a filter. It would be useful in case we wanted to do something similar in this section, but I've removed it for now.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:771
>> +        let expandAllSections = () => {
> 
> This function could be inlined, as it's only used once.  Alternatively, you could make this into a member function and share it between `_layoutStatisticsSection` and `_layoutSourcesSection`.

It requires saving the `statistics` into a member variable, but I may do that to convert some `updateLayout(Internal)` to `updateStatisiticsAndSources`.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:839
>> +        function firstNonNativeCallFrame(callFrames) {
> 
> This looks like it does the same thing as `WI.TimelineRecord.prototype.initiatorCallFrame`.  Can you use that instead?

Yep, nice!

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:852
>> +            return sourceCodeLocation.sourceCode.url + ":" + sourceCodeLocation.lineNumber + ":" + sourceCodeLocation.columnNumber;
> 
> `sourceCodeLocation.originalLocationString(WI.SourceCodeLocation.ColumnStyle.Shown, WI.SourceCodeLocation.NameStyle.Short)`?

This does a lot more work, this is getting called potentially thousands of times so I want it to be simple.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:919
>> +                    entry.count += count;
> 
> Just making sure I understand; is it guaranteed that a repeating timer will only appear in `repeatingTimers` once (e.g. I'm worried about double counting because of the `+=`).

There are two lists here:

    • timerMap shows timers installations in the selected time range
        - this will only ever happen once per timer id, and we are ignoring timer ids overflowing

    • repeatingTimers only shows timers have fired multiple times in the selected time range
        - this will only ever have values for a timer that fired multiple times, not a timer that fired once


If we saw a repeating timer, then its fire count is at least 2 and we try to use that to count as the repeating timer firing N times.

Since Sources is about installations, we aggreate repeated timers to an installation point where possible.

If we have the following:

              setInterval      fire         fire
     time: |       *            *            *    |
  range 1:     |----------------------------------|  => Timer Installed, Repeat of 2     => ~2 (the number of fires of this installed timer in range, with install location)
  range 2:     |----------------------|              => Timer Installed, No Repeat seen  => ~1 (the number of fires of this installed timer in range, with install location)
  range 3:     |------|                              => Timer Installed, No Repeat seen  => ~1 (the number of fires of this installed timer in range, with install location)
  range 4:                  |---------------------|  => No Installed, Repeat of 2        => ~2 (the number of fires of this installed timer in range, with fire location)
  range 5:                   |------|                => No Installed, Fire happened once => nothing (we don't know where it was installed, or where if it is repeating)

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:920
>> +                    entry.repeating = record.details.repeating;
> 
> Should we only be setting this in the `true` case?  IIUC, it's wouldn't be possible for a repeating and non-repeating timer to both be fired from the same location, so this would be "consistent" between iterations.  Should we be asserting as such?

Good point. Apparently it is possible to get `setInterval` and `setTimeout` to have the same location:

    <script>
    function doThing(f) {
        f(function() { console.log("fire") }, 1000);
    }

    doThing(setTimeout);
    doThing(setInterval);
    doThing(setTimeout);
    </script>

In this case they would all be grouped at the same place in `doThing` and not indicated as non-repeating depending on your range.

I'll change this to only go to true and not reset to false:

    if (record.details.repeating)
        entry.repeating = true;

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:971
>> +                let number = entry.repeating ? "~" + entry.count : entry.count;
> 
> Should this be localized?

Good idea.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:975
>> +                let row;
> 
> Style: this should be initialized to some value (e.g. `null`).

It is initialized to undefined and the two branches of the if/else below will initialize it. This is not uncommon style.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:984
>> +                    row.querySelector(".label").append(` ${enDash} ${entry.functionName}`);
> 
> Rather than querying for an element, you could just add this at the end of `label` before you pass it around.
> ```
>     if (entry.functionName)
>         label += ` ${enDash} ${entry.functionName}`;
> ```

Label is not always a string, it is commonly a source code location link. If it was an element I'd want to append a sibling and need to get its parent anyways.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1370
>> +            this._sourcesFilterLabelElement.appendChild(createActiveFilterElement("observer", name));
> 
> Does this do any wrapping or allow scrolling?  If a lot of filters are selected, does that mean it extends past the edge of the window?

Scrolling is allowed.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1417
>> +            row.remove();
> 
> Is this the equivalent of `this._statisticsTable.removeChildren`?

No, the statisticsTable has a static row that doesn't get regenerated ("Script Entries").

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:390
>> +        background-color: gray;
> 
> Ditto (>352).

I don't think it makes much sense to make names for these. In part because we already have ~50 gray variables and I think people already can't tell them apart. Which gray is lighter when inverted or darker?

I've made:

    --gray-background-color
    --gray-foreground-color /* Use with gray background color */

But I think this is getting ridiculous.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:300
>> +            this.updateLayout(WI.CPUTimelineView.LayoutReason.Internal);
> 
> This feels a little like cheating :/  but I can't really find a reason not to like it at the same time :P

This is the <details> element expanding collapsing on user interaction, we can decide to only update layout on open but there won't be a way to synchronize with the UI unless it is synchronous.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:333
>> +        let statisticsContainerElement = createChartContainer(bottomOverviewElement, WI.UIString("Statistics"), "");
> 
> This section is pretty self-explanatory. I'd prefer to omit the subtitle here.

I'll look into this later when making the bottom section flex a bit better. I tried without titles and didn't like it.

[Joseph Pecoraro]

Created attachment 363725 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 363726 [details]
[DIFF] Addressed Review Comments

Posted a diff if that is easier.

[Devin Rousso]

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

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

>>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:300
>>> +            this.updateLayout(WI.CPUTimelineView.LayoutReason.Internal);
>> 
>> This feels a little like cheating :/  but I can't really find a reason not to like it at the same time :P
> 
> This is the <details> element expanding collapsing on user interaction, we can decide to only update layout on open but there won't be a way to synchronize with the UI unless it is synchronous.

Good point!  I still think this is a valid comment against the other cases though.  <https://webkit.org/b/195357>

>>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:327
>>> +            observer: new Set,
>> 
>> Rather than having these keys be something you have to remember, we could leverage computed keys and a static object.
>> ```
>>     this._sourcesFilter = {};
>>     for (let type in Object.values(WI.CPUTimelineView.Source))
>>         this._sourceFilter[type] = new Set;
>> 
>>     WI.CPUTimelineView.Source = {
>>         Timer: "timer",
>>         Event: "event",
>>         Observer: "observer",
>>     };
>> ```
> 
> This seems rather overkill. Is there any advantage?

The main advantage would be not having to hardcode/remember particular strings (e.g. `"timer"`) since you'd be able to use the variable instead.  It's much the same as the `Event` objects.  I also believe that it would be much easier to search for in the codebase, vs something more generic like `"event"`.

>>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:759
>>> +            span.addEventListener("mouseup", (event) => {
>> 
>> Why not use `"click"`?
> 
> We do "mouseup" instead of "click" because during a recording the elements change. So the one you mousedown'd on is different from the one you mouseup'd on and click doesn't work, but mouseup does.
> 
> We do have a flicker issue with mouse hovering these during an active recording already.

If that's the case, can you please file a bug for the flickering issue and link it somewhere in this patch (or just relate it to this bug)?  I think we should try to move this to "click" alongside that fix.

>>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:833
>>> +    _layoutSourcesSection(statistics)
>> 
>> Long method names don't wrap and can add undesired horizontal scrolling for the content view. I suggest wrapping method names.
> 
> We are planning on making this into a CSS grid, which will allow us to use `text-overflow: ellipsis;`.

<https://webkit.org/b/195356>

>>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:984
>>> +                    row.querySelector(".label").append(` ${enDash} ${entry.functionName}`);
>> 
>> Rather than querying for an element, you could just add this at the end of `label` before you pass it around.
>> ```
>>     if (entry.functionName)
>>         label += ` ${enDash} ${entry.functionName}`;
>> ```
> 
> Label is not always a string, it is commonly a source code location link. If it was an element I'd want to append a sibling and need to get its parent anyways.

If that's the case, can you use `document.createDocumentFragment()` instead?
```
    let label = document.createDocumentFragment();
    label.append(labelForLocation(key, sourceCodeLocation);
    if (entry.functionName)
        label.append(` ${enDash} ${entry.functionName}`);
```

[Devin Rousso]

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

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

r=me

I still think we should try to move away from calling `updateLayout` (with the exception of the "toggle" event), but in the interest of getting this landed for further features we can do that in a followup (not to mention this is fully working in the state it's in right now).

I've created two followup bugs:
 - <https://webkit.org/b/195356> Web Inspector: CPU Usage Timeline - leverage CSS grid for Statistics and Sources sections
 - <https://webkit.org/b/195357> Web Inspector: CPU Usage Timeline - don't eagerly render the Statistics and Sources sections

> LayoutTests/inspector/unit-tests/map-utilities.html:8
> +    let suite = InspectorTest.createSyncSuite("MapUtilities");

NIT: I'd just call this `Map` so that it matches as a prefix for the individual test cases.

If you were feeling like it, you could also write tests for `Map.fromObject` and `Map.prototype.take` :D

> LayoutTests/inspector/unit-tests/map-utilities.html:24
> +            return true;

NIT: I've come to learn that this isn't actually necessary for a sync suite.  You can omit a return and it assumes a pass.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:395
> +        background-color: hsl(0, 0%, 33%);

`var(--gray-foreground-color)`?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:338
> +        let statisticsContainerElement = createChartContainer(bottomOverviewElement, WI.UIString("Statistics"), "");

Rather than have an empty string, could you modify `createChartContainer` to only set the `title` if a string is actually provided?  It looks weird to have a falsy value hanging off the end of a function call.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:355
> +        let sourcesContainerElement = createChartContainer(bottomOverviewElement, WI.UIString("Sources"), "");

Ditto (>338).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:745
> +    _layoutStatisticsAndSources()

Personally, I think this is a bit unnecessary, but considering how often it's called, I can see why it is nice.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:795
> +            let entries = Array.from(map.entries());

You can drop the `.entries()`.  The default iterator for a `Map` is the same as calling `.entries()`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1411
> +    _insertTableRow(table, rowList, heading, numberValue, labelValue, followingRow = null)

Was there a reason you didn't change this to be an optional object?  All of the places where you pass `""` as a parameter are pretty confusing to follow, to the point where I actually had to count out the arguments to understand which argument they corresponded to.  I think we should avoid cases where we pass inlined falsy values to functions, as they are often very hard to follow.  It's even more confusing given that one of the parameters actually is optional (`followingRow`).  If you're worried about performance, then I understand not wanting to use a temporary object, but at the very least reorder the parameters and make so that the optional values (e.g. `heading` and `labelValue`) don't have to be supplied in any way when not needed (at the same time, there's no real reason to have `followingRow` have a default value either, since `undefined` and `null` are both falsy).

[Joseph Pecoraro]

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

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

>> LayoutTests/inspector/unit-tests/map-utilities.html:24
>> +            return true;
> 
> NIT: I've come to learn that this isn't actually necessary for a sync suite.  You can omit a return and it assumes a pass.

Oh snap!

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:395
>> +        background-color: hsl(0, 0%, 33%);
> 
> `var(--gray-foreground-color)`?

It is incidental that this matches gray-foreground-color. It is the opposite of the `hsl(0, 0%, 85)` which is never the gray-background-color.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:795
>> +            let entries = Array.from(map.entries());
> 
> You can drop the `.entries()`.  The default iterator for a `Map` is the same as calling `.entries()`.

Oh smart!

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1411
>> +    _insertTableRow(table, rowList, heading, numberValue, labelValue, followingRow = null)
> 
> Was there a reason you didn't change this to be an optional object?  All of the places where you pass `""` as a parameter are pretty confusing to follow, to the point where I actually had to count out the arguments to understand which argument they corresponded to.  I think we should avoid cases where we pass inlined falsy values to functions, as they are often very hard to follow.  It's even more confusing given that one of the parameters actually is optional (`followingRow`).  If you're worried about performance, then I understand not wanting to use a temporary object, but at the very least reorder the parameters and make so that the optional values (e.g. `heading` and `labelValue`) don't have to be supplied in any way when not needed (at the same time, there's no real reason to have `followingRow` have a default value either, since `undefined` and `null` are both falsy).

Just got lost in all the comments. I'll update. One thing that would help with comment overload is to not include as many ditto lines if they are obvious repeated comments. Since everything in this patch is done approximately 3 times in a row, repeating comments 3 times right in a row just adds clutter.

Performance isn't as much of an issue here since we tend to insert ~20 rows or so and not hundreds. It is also possible in cases like these (where the caller and receiver construct and deconstruct an object) that JavaScriptCore may be able to eliminate the object allocation entirely.

I also made the last property not have a `= {}` since I want to make sure every caller passes in an object.

[Joseph Pecoraro]

https://trac.webkit.org/r242562

[Radar WebKit Bug Importer]

<rdar://problem/48648540>

[Nikita Vasilyev]

I'm late with my comments, but I'll mention it anyway.

(In reply to Joseph Pecoraro from comment #16)
> Comment on attachment 363404 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363404&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:390
> >> +        background-color: gray;
> > 
> > Ditto (>352).
> 
> I don't think it makes much sense to make names for these. In part because
> we already have ~50 gray variables and I think people already can't tell
> them apart. Which gray is lighter when inverted or darker?
> 
> I've made:
> 
>     --gray-background-color
>     --gray-foreground-color /* Use with gray background color */

1. I don't think we should define these colors in Variables.css. It's like defining a global variable that is only used by one view. For this case, I'd keep the variables in CPUTimelineView.css.

2. We should use semantic color whenever possible, e.g. avoid putting "gray" in a variable name. As you mentioned, having ~50 gray variables is very confusing.

[Joseph Pecoraro]

> > I've made:
> > 
> >     --gray-background-color
> >     --gray-foreground-color /* Use with gray background color */
> 
> 1. I don't think we should define these colors in Variables.css. It's like
> defining a global variable that is only used by one view. For this case, I'd
> keep the variables in CPUTimelineView.css.
> 
> 2. We should use semantic color whenever possible, e.g. avoid putting "gray"
> in a variable name. As you mentioned, having ~50 gray variables is very
> confusing.

Yep, I agree in both cases for the exact same reasons.