Summary: | Web Inspector: CPU Usage Timeline - Main Thread Indicator | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||||
Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | hi, inspector-bugzilla-changes, joepeck, nvasilyev, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 194455 | ||||||||||||
Attachments: |
|
Description
Joseph Pecoraro
2019-02-22 17:23:24 PST
Created attachment 362802 [details]
[PATCH] Proposed Fix
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 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. Created attachment 362960 [details]
[PATCH] Proposed Fix
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. Created attachment 362961 [details]
[PATCH] Proposed Fix
Created attachment 362964 [details]
[PATCH] Proposed Fix
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`. |