[webkit-reviews] review granted: [Bug 195321] Web Inspector: CPU Usage Timeline - Allow clicking a bar in the overview to select a tight time range around it : [Attachment 363619] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 195321: Web Inspector: CPU Usage Timeline - Allow clicking a bar in the
overview to select a tight time range around it
https://bugs.webkit.org/show_bug.cgi?id=195321

Attachment 363619: [PATCH] Proposed Fix

https://bugs.webkit.org/attachment.cgi?id=363619&action=review




--- Comment #2 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 363619
  --> https://bugs.webkit.org/attachment.cgi?id=363619
[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>").