Bug 195321

Created attachment 363619 [details] [PATCH] Proposed Fix

Comment on attachment 363619 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=363619&action=review r=me > Source/WebInspectorUI/UserInterface/Models/Timeline.js:113 > + if (before < after) > + return recordBefore; > + return recordAfter; NIT: I'd make this into a ternary. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.css:85 > + fill: var(--selected-background-color) !important; > + stroke: var(--selected-background-color-active) !important; Is there any way to avoid the `!important`? :( > Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.css:86 > + fill-opacity: 0.5; NIT: I'd put this alongside the `fill` property above. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:49 > + this._chart.element.addEventListener("click", this._handleGraphMouseClick.bind(this)); NIT: I'd use `_handleChartClick` instead, as it more closely matches the variable name ("Chart" vs "Graph") and event name ("Click vs "MouseClick"). NIT: I'd move this above to be immediately below the initialization of `this._chart`. I prefer adding event listeners before adding the element to the DOM. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:54 > + this._lastSelectedRecordLayout = null; Why is "Layout" part of the name? It seems unnecessary. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:119 > + let visibleRecords = this._cpuTimeline.recordsInTimeRange(graphStartTime, visibleEndTime + (WI.CPUTimelineOverviewGraph.samplingRatePerSecond / 2), includeRecordBeforeStart); I thought you preferred using the non-`WI` name when referenced from within the class? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:127 > + let intervalWidth = (WI.CPUTimelineOverviewGraph.samplingRatePerSecond / secondsPerPixel); Ditto (>119). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:134 > + let x = xScale(record.startTime - (WI.CPUTimelineOverviewGraph.samplingRatePerSecond / 2)); Ditto (>119). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:150 > + if (this._lastSelectedRecordLayout !== this.selectedRecord) > + this.needsLayout(); It might be worth mentioning that this is necessary since we don't actually have any of the actual DOM nodes for the records. As such, the only way to add a "selected" class to a matching record is by redrawing the entire graph. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:180 > + // Try again lower, since the scroll bar prevents low clicks. > + const tryLowerOffset = 5; > + elements = document.elementsFromPoint(event.pageX, event.pageY + tryLowerOffset); > + rectElement = elements.find((x) => x.localName === "rect"); Do we still need this now that <https://webkit.org/b/195318> has landed? > Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:185 > + let chartElement = rectElement.closest(".stacked-column-chart"); It's cases like these that I see the utility in having a static variable for the CSS class of the element (e.g. `WI.StackedColumnChart.className`). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:194 > + let rect = chartElement.getBoundingClientRect(); > + let position = event.pageX - rect.left; > + > + if (WI.resolvedLayoutDirection() === WI.LayoutDirection.RTL) > + return rect.width - position; > + return position; We should make this into a utility function on `Element.prototype` given how often we seem to use it. Something like `positionWithinElement`. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:199 > + let clickPosition = this._graphPositionForMouseEvent(event); Shouldn't this be called `graphPosition` to match the function name? If not, we should rename it to `_clickPositionForMouseEvent` or something like that. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:203 > + let secondsPerPixel = this.timelineOverview.secondsPerPixel; NIT: I'd inline this. > Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:207 > + let clickTime = graphStartTime + graphClickTime; Ditto (>203). > Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:209 > + console.log("click", record); Oops. > Source/WebInspectorUI/UserInterface/Views/StackedColumnChart.js:87 > + addColumnSet(x, totalHeight, width, heights, additionalClass) It's moments like these that I wish we used `optional = {}` in more places :( > Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:770 > + let selectionPadding = 2.25 * WI.CPUTimelineOverviewGraph.samplingRatePerSecond; NIT: I'd flip the order of the multiplication so that the "base" is first and the "multiplier" is second (e.g. "multiply <known value> by <scale>" rather than "multiply <scale> by <known value>").

Comment on attachment 363619 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=363619&action=review >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.css:85 >> + stroke: var(--selected-background-color-active) !important; > > Is there any way to avoid the `!important`? :( I was matching what we did for timeline record bars: .timeline-record-bar.selected > .segment { background-color: var(--selected-background-color) !important; border-color: var(--selected-background-color-active) !important; } Normally "selected" appearances are the rare cases we use important to ensure regardless of specificity we get selected styles. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:54 >> + this._lastSelectedRecordLayout = null; > > Why is "Layout" part of the name? It seems unnecessary. It is the last selected record we performed a layout with. Selected record may change many times, but we only need to update our layout if it doesn't match the one when we last did a layout. I'll rename this to: this._lastSelectedRecordInLayout >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:119 >> + let visibleRecords = this._cpuTimeline.recordsInTimeRange(graphStartTime, visibleEndTime + (WI.CPUTimelineOverviewGraph.samplingRatePerSecond / 2), includeRecordBeforeStart); > > I thought you preferred using the non-`WI` name when referenced from within the class? Oops, yes I meant to go back and fix this after the global replace I did. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:150 >> + this.needsLayout(); > > It might be worth mentioning that this is necessary since we don't actually have any of the actual DOM nodes for the records. As such, the only way to add a "selected" class to a matching record is by redrawing the entire graph. Added a comment. >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:180 >> + rectElement = elements.find((x) => x.localName === "rect"); > > Do we still need this now that <https://webkit.org/b/195318> has landed? Hmm, yes and no. When there is no scrollbar visible, clicks will work everywhere now. But if there is a scroll bar visible then there might be a bar underneath the scrollbar (an overlay) which cannot be clicked directly. That said, I don't think scrollbars are common here, and normally I think people will want to click large columns, not near zero columns (which are already hard to click at 4px). >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:199 >> + let clickPosition = this._graphPositionForMouseEvent(event); > > Shouldn't this be called `graphPosition` to match the function name? If not, we should rename it to `_clickPositionForMouseEvent` or something like that. I'll just name this `position`. It is the click position within the graph.

<rdar://problem/48653647>