[webkit-reviews] review granted: [Bug 193730] Web Inspector: CPU Usage Timeline : [Attachment 359929] [PATCH] Proposed Fix
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jan 23 13:57:14 PST 2019
Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 193730: Web Inspector: CPU Usage Timeline
https://bugs.webkit.org/show_bug.cgi?id=193730
Attachment 359929: [PATCH] Proposed Fix
https://bugs.webkit.org/attachment.cgi?id=359929&action=review
--- 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?
More information about the webkit-reviews
mailing list