Bug 194972 – Web Inspector: CPU Usage Timeline - Main Thread Indicator

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

CPU Usage Timeline - Main Thread Indicator

Add a strip in the CPU Timeline Graph that highlights what the main thread was doing.
Clicking within it selects the relevant timeline events.

[Joseph Pecoraro]

Created attachment 362802 [details]
[PATCH] Proposed Fix

[Nikita Vasilyev]

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

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

> Source/WebInspectorUI/ChangeLog:12
> +        The existing enclosingNode doesn't work for SVG because its names
> +        are lowercase. Add a simplified version for the svg case.

I think we should fix the existing enclosingNodeOrSelf instead of introducing a new method.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:213
> Object.defineProperty(Node.prototype, "enclosingNodeOrSelfWithNodeName",
> {
>     value(nodeName)
>     {
>         return this.enclosingNodeOrSelfWithNodeNameInArray([nodeName]);

enclosingNodeOrSelfWithNodeNameInArray is only used here and in one place in TreeOutline:

        var listNode = node.enclosingNodeOrSelfWithNodeNameInArray(["ol", "li"]);

I think we should get rid of enclosingNodeOrSelfWithNodeNameInArray. It's a 9-word method that's really needed only in one place.

[Devin Rousso]

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

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

> Source/WebInspectorUI/ChangeLog:45
> +        Place the Main Thread Indicator view beneath the big graph.

NIT: s/Place/Move

> Source/WebInspectorUI/ChangeLog:46
> +        Clicks inside it select records in the Timeline Overview.

NIT: s/Clicks/Clicking and s/select/selects

> Source/WebInspectorUI/UserInterface/Models/Timeline.js:96
> +    recordsOverlappingTimeRange(startTime, endTime)

It sucks that we need a separate call for this.  I'd almost rather have the callers that specifically need the current functionality of `recordsInTimeRange` do their own filtering at the call site :(

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:678
> +        let svgRect = svgElement.getBoundingClientRect();
> +        let position = event.pageX - svgRect.left;

If all you need is the `left`, you could use `totalOffsetLeft` instead.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:681
> +            return svgRect.width - position;

This should be `offsetWidth` (computed size in pixels), not `width` (SVG attribute value).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:710
> +            if (this._attemptSelectIndicatatorTimelineRecord(clickStartTime, clickStartTime + delta))

This should be `clickEndTime + delta`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:725
> +            case WI.LayoutTimelineRecord.EventType.RecalculateStyles:
> +            case WI.LayoutTimelineRecord.EventType.ForcedLayout:
> +            case WI.LayoutTimelineRecord.EventType.Layout:
> +            case WI.LayoutTimelineRecord.EventType.Paint:
> +            case WI.LayoutTimelineRecord.EventType.Composite:

A comment explaining that "these types of records are the only ones that do any work" would be nice :)

You could also remove the other `case`s and just `return false;` since we don't "care" about them anyways (unless you want to make sure new record types get "considered", in which case there should be an assertion/error).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:748
> +            case WI.ScriptTimelineRecord.EventType.ScriptEvaluated:
> +            case WI.ScriptTimelineRecord.EventType.APIScriptEvaluated:
> +            case WI.ScriptTimelineRecord.EventType.ObserverCallback:
> +            case WI.ScriptTimelineRecord.EventType.EventDispatched:
> +            case WI.ScriptTimelineRecord.EventType.MicrotaskDispatched:
> +            case WI.ScriptTimelineRecord.EventType.TimerFired:
> +            case WI.ScriptTimelineRecord.EventType.AnimationFrameFired:

Ditto (>721).

> Source/WebInspectorUI/UserInterface/Views/CPUUsageIndicatorView.css:37
> +    --cpu-usage-view-details-border-end: 1px solid var(--border-color);

Style: I add another newline before CSS variables to help distinguish what is an actual property vs what is just a "holder of a value".

> Source/WebInspectorUI/UserInterface/Views/CPUUsageIndicatorView.css:41
> +    border-right: var(--cpu-usage-view-details-border-end);

`-webkit-border-end`?

> Source/WebInspectorUI/UserInterface/Views/CPUUsageIndicatorView.css:55
> +    z-index: calc(var(--timeline-marker-z-index) + 1);

NIT: I normally put `z-index` alongside positional properties (e.g. `top`).

> Source/WebInspectorUI/UserInterface/Views/CPUUsageIndicatorView.js:32
> +        this.element.classList.add("cpu-usage-indicator-view");

NIT: it seems like all of the logic in this constructor can be moved to an `initialLayout`.

> Source/WebInspectorUI/UserInterface/Views/CPUUsageIndicatorView.js:72
> +            let type = samples[i];

NIT: it seems odd that we're getting a `type` from a `sample`, but I can't think of better names.

> Source/WebInspectorUI/UserInterface/Views/CPUUsageIndicatorView.js:88
> +            // If changed type, close current chunk.

NIT: s/changed type/type changed

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:796
> +    _recordWasSelected(event)

There already is a `_recordSelected` event handler in this class.  I think we should rename that event to `_handleTimelineOverviewRecordSelected` and name this new function `_handleTimelineViewRecordSelected`.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:802
> +

Style: unnecessary newline.

[Joseph Pecoraro]

Created attachment 362960 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/ChangeLog:45
>> +        Place the Main Thread Indicator view beneath the big graph.
> 
> NIT: s/Place/Move

It did not exist before, so we are placing it now.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:681
>> +            return svgRect.width - position;
> 
> This should be `offsetWidth` (computed size in pixels), not `width` (SVG attribute value).

This should be the getBoundingClientRect() rect, to it won't have a offsetWidth property and width should be that.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:725
>> +            case WI.LayoutTimelineRecord.EventType.Composite:
> 
> A comment explaining that "these types of records are the only ones that do any work" would be nice :)
> 
> You could also remove the other `case`s and just `return false;` since we don't "care" about them anyways (unless you want to make sure new record types get "considered", in which case there should be an assertion/error).

I want to handle them all to make it clear if we miss a new case in the future. I'll keep the comments in the previous patch as the reference for this file.

[Joseph Pecoraro]

Created attachment 362961 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 362964 [details]
[PATCH] Proposed Fix

[Devin Rousso]

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

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

r=me

I think this might need a rebase, as bugzilla wasn't able to apply it during the review :(

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:221
> +Object.defineProperty(Node.prototype, "enclosingNodeOrSelfWithLocalName",

I agree with @Nikita.  I think we should fix the existing version, rather than create a new one.

Considering that we `toUpperCase` the `nodeNames`, we should be able to just `toLowerCase` the `node.nodeName` instead.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:208
> +        this._mainThreadWorkIndicatorView.chart.element.addEventListener("click", this._handleIndicatorClick.bind(this));

This could use a more "specific" name, like `_handleMainThreadWorkIndicatorClick`.

[Joseph Pecoraro]

https://trac.webkit.org/r242104

[Radar WebKit Bug Importer]

<rdar://problem/48413488>