Bug 194972 - Web Inspector: CPU Usage Timeline - Main Thread Indicator
Summary: Web Inspector: CPU Usage Timeline - Main Thread Indicator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks: 194455
  Show dependency treegraph
 
Reported: 2019-02-22 17:23 PST by Joseph Pecoraro
Modified: 2019-02-26 13:58 PST (History)
5 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (30.43 KB, patch)
2019-02-22 17:29 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (31.12 KB, patch)
2019-02-25 21:05 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (31.12 KB, patch)
2019-02-25 21:59 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (31.12 KB, patch)
2019-02-25 22:38 PST, Joseph Pecoraro
drousso: review+
drousso: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-02-22 17:23:24 PST
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.
Comment 1 Joseph Pecoraro 2019-02-22 17:29:33 PST
Created attachment 362802 [details]
[PATCH] Proposed Fix
Comment 2 Nikita Vasilyev 2019-02-22 17:42:34 PST
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.
Comment 3 Devin Rousso 2019-02-25 17:08:15 PST
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.
Comment 4 Joseph Pecoraro 2019-02-25 21:05:53 PST
Created attachment 362960 [details]
[PATCH] Proposed Fix
Comment 5 Joseph Pecoraro 2019-02-25 21:10:38 PST
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.
Comment 6 Joseph Pecoraro 2019-02-25 21:59:39 PST
Created attachment 362961 [details]
[PATCH] Proposed Fix
Comment 7 Joseph Pecoraro 2019-02-25 22:38:11 PST
Created attachment 362964 [details]
[PATCH] Proposed Fix
Comment 8 Devin Rousso 2019-02-26 00:21:21 PST
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`.
Comment 9 Joseph Pecoraro 2019-02-26 13:41:12 PST
https://trac.webkit.org/r242104
Comment 10 Radar WebKit Bug Importer 2019-02-26 13:58:53 PST
<rdar://problem/48413488>