RPP: deal with time range overlaps [devtools/devtools-frontend : main]

0 views

Skip to first unread message

Jack Franklin (Gerrit)

unread,

Jun 21, 2024, 10:01:54 AM (9 days ago) Jun 21

to Alina Varkki, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Attention needed from Alina Varkki

Jack Franklin voted Auto-Submit+1

Auto-Submit

Open in Gerrit

Related details

Attention is currently required from:

Alina Varkki

Submit Requirements:

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Alina Varkki (Gerrit)

unread,

Jun 27, 2024, 4:21:08 PM (3 days ago) Jun 27

to Jack Franklin, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Attention needed from Jack Franklin

Alina Varkki voted and added 3 comments

Votes added by Alina Varkki

Code-Review

3 comments

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Alina Varkki . resolved

LGTM. Good algorithm 😎

File front_end/panels/timeline/Overlays.ts

Line 316, Patchset 3 (Latest): if (timeRangeOverlays.length) {

Alina Varkki . unresolved

We don't need to check for overlaps if there is only 1 range

Line 374, Patchset 3 (Latest): element?.classList.add(`overlap-${firstIndexForOverlapClass++}`);

Alina Varkki . unresolved

If a range has 2 overlapping ranges, it doesn't mean that they will also overlap each other so we might be increasing the elevation unnecessarily.

I know we don't want to spend too much time on this right now so let's ship it and keep this in mind.

Open in Gerrit

Related details

Attention is currently required from:

Jack Franklin

Submit Requirements:

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages