[Webkit-unassigned] [Bug 193730] Web Inspector: CPU Usage Timeline

Wed Jan 23 13:57:14 PST 2019

https://bugs.webkit.org/show_bug.cgi?id=193730

Devin Rousso <drousso at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |drousso at apple.com
 Attachment #359929|review?                     |review+
              Flags|                            |

--- Comment #9 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 359929
  --> https://bugs.webkit.org/attachment.cgi?id=359929
[PATCH] Proposed Fix

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

r=me, awesome work!  I love the colors.

I figured out towards the end that you'd used most of the Memory code to create this (since they're pretty similar), but I think we can take a little time to clean it up a bit, so I'm leaving my NITs.

> LayoutTests/inspector/cpu-profiler/tracking.html:8
> +    let suite = ProtocolTest.createAsyncSuite("CPUProfiler.startTracking and CPUProfiler.stopTracking");

NIT: I like to have the suite name closely match the path to the test itself:

    let suite = ProtocolTest.createAsyncSuite("CPUProfiler.Tracking");
    ...
    suite.addTestCase({
        name: "CPUProfiler.Tracking.Event",
        ...
    });

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

This id seems very "generic".  Is there a reason to have this be a special type rather than just inline the two values?  Considering that neither of them is optional, it seems like it would be easier/simpler to just have them as direct parameters of "trackingUpdate".

Alternatively, maybe just a better name other than "Event" would be good.  How about "TimelineRecord" or just "Record"?

> Source/JavaScriptCore/inspector/protocol/CPUProfiler.json:28
> +            "name": "trackingStart",

NIT: "trackingStarted", since this event is fired after tracking has started.

> Source/JavaScriptCore/inspector/protocol/CPUProfiler.json:42
> +            "name": "trackingComplete",

NIT: "trackingStopped" to better match the corresponding command "stopTracking".

> Source/WebCore/inspector/agents/InspectorCPUProfilerAgent.cpp:59
> +    if (m_tracking)

NIT: this would be a good place to use `errorString`, like "Already started tracking".

> Source/WebCore/inspector/agents/InspectorCPUProfilerAgent.cpp:73
> +    if (!m_tracking)

NIT: this would be a good place to use `errorString`, like "Already stopped tracking".

> Source/WebInspectorUI/UserInterface/Images/CPUInstrument.svg:2
> +<!-- Copyright Â© 2016 Apple Inc. All rights reserved. -->

2019?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.css:27
> +body .sidebar > .panel.navigation.timeline > .timelines-content li.item.cpu,
> +body .timeline-overview > .graphs-container > .timeline-overview-graph.cpu {

Is the `body` part of the selector necessary?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.css:46
> +    right: 0;

NIT: although this value is 0, I think our preferred style is to still use a variable.

    --cpu-timeline-overview-graph-legend-offset-end: 0;

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:34
> +        console.assert(timeline instanceof WI.Timeline);

NIT: I normally put assertions for parameters above the `super` call, so that they appear before any other assertions (e.g. from super classes).
NIT: is it worth asserting that the type of the timeline is as expected?

    console.assert(timeline.type === WI.TimelineRecord.Type.CPU);

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:37
> +        this._cpuTimeline.addEventListener(WI.Timeline.Event.RecordAdded, this._cpuTimelineRecordAdded, this);

NIT: I like to prefix event handlers with `_handle...` so it's immediately clear as to how the function is expected to be used.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:39
> +        let size = new WI.Size(0, this.height);

0 width?  Does this mean we show nothing if "CPU" is selected and we have yet to start a recording?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:51
> +    get height()

I don't really like this name, as it implies that the height of the entire "overview graph" is 72, which isn't necessarily true.  Could you save `size` to a member variable, and just update it as needed?

    this._size = new WI.Size(0, 72);
    ...
    this._size.width = ...

Or (to re-use what you do later), just use the size from the chart itself.

    this._chart.size = new WI.Size(..., this._chart.size.height);

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:88
> +        // 500ms. This matches the ResourceUsageThread sampling frequency in the backend.

Should we add a comment inside `WebCore::ResourceUsageThread` that the `500_ms` should stay in sync with this value?  Are we ever expecting that value to change?  If so, should we care about dealing with differing values between the frontend/backend (e.g. backend changes to poll at 250_ms, and the frontend expects 500_ms)?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:101
> +        let visibleRecords = this._cpuTimeline.recordsInTimeRange(graphStartTime, visibleEndTime + (samplingRatePerSecond / 2), true);

NIT: replace `true` with `includeRecordBeforeStart`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:128
> +        if (this._cachedMaxUsage === this._maxUsage)

Is this purely to avoid re-drawing the legend if the value hasn't changed?  It seems like `this._cachedMaxUsage` isn't used otherwise.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:36
> +    font-family: '-webkit-system-font';

NIT: are the quotes necessary?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:35
> +        console.assert(timeline.type === WI.TimelineRecord.Type.CPU, timeline);

Ditto (>CPUTimelineOverviewGraph.js:34).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:42
> +        // FIXME: Overview with charts.

Are you referring to pie-charts, like the Memory timeline?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:58
> +        timeline.addEventListener(WI.Timeline.Event.RecordAdded, this._cpuTimelineRecordAdded, this);

Ditto (>CPUTimelineOverviewGraph.js:37).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:73
> +    hidden()
> +    {
> +        super.hidden();
> +    }

Remove?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:142
> +            if (discontinuity) {

This can be merged with the `if` above, as well as moving `discontinuity` to be inside that `if`.

> Source/WebInspectorUI/UserInterface/Views/CPUUsageView.css:34
> +    font-family: '-webkit-system-font';

Ditto (>CPUTimelineView.css:36).

> Source/WebInspectorUI/UserInterface/Views/CPUUsageView.css:36
> +    color: hsl(0, 0%, 70%);

How does this look in dark mode?

> Source/WebInspectorUI/UserInterface/Views/CPUUsageView.css:38
> +    --cpu-usage-view-details-padding-start: 15px;

You can simplify this with `-webkit-padding-start`.

> Source/WebInspectorUI/UserInterface/Views/CPUUsageView.js:26
> +WI.CPUUsageView = class CPUUsageView extends WI.Object

Does this need `WI.Object`?

> Source/WebInspectorUI/UserInterface/Views/ColumnChart.js:62
> +    get bars() { return this._bars; }

NIT: this is never used.

> Source/WebInspectorUI/UserInterface/Views/ColumnChart.js:106
> +            let g = this._svgElement.appendChild(createSVGElement("g"));

Can you apply the `transform` (or just use `x` and `y` attributes) directly to the rect, rather than create another element?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190123/3d3ea7be/attachment-0001.html>