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

[bugzilla-daemon at webkit.org]

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

--- Comment #15 from Joseph Pecoraro <joepeck at webkit.org> ---
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

>> 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"?

It is intended to be generic. This is the same pattern as the Memory and ScriptProfiler domains (two others that follow the trackingStart, trackingUpdate, trackingComplete pattern).

>> Source/JavaScriptCore/inspector/protocol/CPUProfiler.json:42
>> +            "name": "trackingComplete",
> 
> NIT: "trackingStopped" to better match the corresponding command "stopTracking".

These names match the existing patterning other domains.

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

Yes, but the size is always updated in layout.

>> 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);

This *is* the height of the entire TimelineOverviewGraph. This is overriding the default value in the parent which uses it for that purpose.

>> 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)?

Good idea.

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

Yes. The cost of resetting the value to the same thing was a measurable perf issue in Timelines in the past.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:36
>> +    font-family: '-webkit-system-font';
> 
> NIT: are the quotes necessary?

Good point, I changed all instances to:

    font-family: -webkit-system-font, sans-serif;

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:42
>> +        // FIXME: Overview with charts.
> 
> Are you referring to pie-charts, like the Memory timeline?

Yes, some overview charts with extra detail is the goal.

>> Source/WebInspectorUI/UserInterface/Views/CPUUsageView.css:36
>> +    color: hsl(0, 0%, 70%);
> 
> How does this look in dark mode?

We look good in dark mode! NVI has some gray color suggestions I'll change in a follow-up.

>> Source/WebInspectorUI/UserInterface/Views/CPUUsageView.js:26
>> +WI.CPUUsageView = class CPUUsageView extends WI.Object
> 
> Does this need `WI.Object`?

Nope!

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

Yep, but adding this to match LineChart.

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

Yes! Eliminated the <g>!

> Source/WebInspectorUI/UserInterface/Views/Variables.css:120
> +    --cpu-stroke-color: hsl(167, 63%, 33%);
> +    --cpu-fill-color: hsl(118, 43%, 54%);

I tweaked these styles so that the stroke doesn't change the hue (this value was more blue then the fill color, the new value stays the same green hue and adjusts the other values).

-- 
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/20190124/5a754772/attachment-0001.html>