Bug 236050 – Web Inspector: When selecting timeline records from a coalesced record bar, select the record nearest the cursor, not the first record in the group

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

<rdar://78629845>

[Patrick Angle]

Created attachment 450722 [details]
Patch v1.0

[Devin Rousso]

Comment on attachment 450722 [details]
Patch v1.0

View in context: https://bugs.webkit.org/attachment.cgi?id=450722&action=review

r=me, nice change :)

tho I am slightly concerned that this behavior might be lost on developers, as there's really no visual indication of this anywhere :/

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:288
>          var barDuration = barEndTime - barStartTime;

NIT: Can we replace this local variable with `this._cachedBarDuration` so we don't have a duplicate?

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:429
> +        if (!this._delegate?.timelineRecordBarClicked || !this._cachedBarDuration)

NIT: I'd separate this into two `if` so that the `_delegate` logic isn't smushed up with some other internal state logic

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:433
> +        if (this._records.length === 1)
> +            this._delegate.timelineRecordBarClicked(this._records[0]);

Is there a missing `return` here?

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:443
> +                this._delegate.timelineRecordBarClicked(record);
> +                return;

NIT: Instead of having another `return`, you could `closestReecord = record;` and then just `break`; so there's only one path to exit this function (i.e. it runs to completion) instead of two.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:447
> +            if (timeBetweenRecordAndTargetTime < closestRecordTimeDelta) {

NIT: While this approach does look like it'd work, I think it may be more work than actually needed.  Can we instead just find the closest record before and after the `targetRecordTime` and return whichever of those two is closer?  As an example, if a combined record bar has four evenly spaced (sub)records and the developer clicks in the middle, there's really no reason to even consider the first and last (sub)records since there's another (sub)record after and before each respectively.  At the very least you could `break` out of this loop once you've reached the first (sub)record after the `targetRecordTime`.

[Patrick Angle]

Pull request: https://github.com/WebKit/WebKit/pull/3900

[EWS]

Committed 254056@main (e9a80fbb0fb7): <https://commits.webkit.org/254056@main>

Reviewed commits have been landed. Closing PR #3900 and removing active labels.