[webkit-reviews] review granted: [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 : [Attachment 450722] Patch v1.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
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
https://bugs.webkit.org/show_bug.cgi?id=236050

Attachment 450722: Patch v1.0

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




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