Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/components/timeline/Selection.css
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@
pointer-events: none;
}

.timelineSelectionOverlayTime {
position: absolute;
z-index: 1;
padding: 3.25px 8px;
border-radius: 0 4px 4px 0;
margin-left: 1px;
background-color: rgb(153 153 153);
color: #fff;
}

.timelineSelectionOverlay {
position: absolute;
z-index: 2;
Expand Down
16 changes: 15 additions & 1 deletion src/components/timeline/Selection.js
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,10 @@ class TimelineRulerAndSelection extends React.PureComponent<Props> {
return;
}
const { width, committedRange, changeMouseTimePosition } = this.props;
if (width === 0) {
// This can happen when hovering before the profile is fully loaded.
return;
}

const rect = getContentRect(this._container);
if (
Expand Down Expand Up @@ -399,6 +403,7 @@ class TimelineRulerAndSelection extends React.PureComponent<Props> {
mouseTimePosition,
width,
committedRange,
zeroAt,
} = this.props;

let hoverLocation = null;
Expand Down Expand Up @@ -431,7 +436,16 @@ class TimelineRulerAndSelection extends React.PureComponent<Props> {
: undefined,
left: hoverLocation === null ? '0' : `${hoverLocation}px`,
}}
/>
>
<span className="timelineSelectionOverlayTime">
{mouseTimePosition !== null
? getFormattedTimeLength(
mouseTimePosition - zeroAt,
(committedRange.end - committedRange.start) / width
)
: null}
</span>
</div>
</div>
);
}
Expand Down
10 changes: 7 additions & 3 deletions src/profile-logic/committed-ranges.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
// @flow

import { formatTimestamp } from 'firefox-profiler/utils/format-numbers';
import type { StartEndRange } from 'firefox-profiler/types';
import type { Milliseconds, StartEndRange } from 'firefox-profiler/types';

/**
* Users can make preview range selections on the profile, and then can commit these
Expand Down Expand Up @@ -164,11 +164,15 @@ export function stringifyCommittedRanges(
return arrayValue.map(stringifyStartEnd).join('~');
}

export function getFormattedTimeLength(length: number): string {
export function getFormattedTimeLength(
length: Milliseconds,
precision: Milliseconds = Infinity
): string {
return formatTimestamp(
length,
/*significantdigits*/ 2,
/*maxFractionalDigits*/ 2
/*maxFractionalDigits*/ 2,
precision
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,10 @@ exports[`ActiveTabTimeline should be rendered properly from the Timeline compone
<div
class="timelineSelectionHoverLine"
style="visibility: hidden; left: 0px;"
/>
>
<span
class="timelineSelectionOverlayTime"
/>
</div>
</div>
`;
8 changes: 7 additions & 1 deletion src/test/components/__snapshots__/Timeline.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,13 @@ Process: \\"default\\" (0)"
<div
class="timelineSelectionHoverLine"
style="left: 120px;"
/>
>
<span
class="timelineSelectionOverlayTime"
>
3ms
</span>
</div>
</div>
<nav
class="react-contextmenu timelineTrackContextMenu"
Expand Down
14 changes: 10 additions & 4 deletions src/test/unit/marker-schema.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -220,13 +220,16 @@ describe('marker schema formatting', function () {
['duration', 50000000],
['duration', 123456789],
['duration', 0.000123456],
['duration', 1000 * 60 - 0.01], // slightly less than 1min, should not be shown as '60s'
['duration', 1000 * 60 - 501], // slightly less than 1min, should not be shown as '60s'
['duration', 1000 * 60 - 500], // slightly less than 1min, should not be shown as '60s'
['duration', 1000 * 60 + 1], // slightly more than 1min, should not be shown as '1min0s'
['duration', 779873.639], // '13min60s' should be rounded to '14min'
['duration', 1000 * 3600 - 0.01], // slightly less than 1h, should not be shown as '0h60min'
['duration', 1000 * 3600 - 501], // 1h - 501ms slightly less than 1h, should be shown as '59min59s'
['duration', 1000 * 3600 - 500], // 1h - 500s slightly less than 1h, should be shown as '1h', not '0h60min'
['duration', 1000 * 3600 + 1], // slightly more than 1h, should not be shown as '1h0min'
['duration', 1000 * 3600 * 24 - 0.01], // slightly less than 1 day, should not be shown as '0d24h'
['duration', 1000 * 3600 * 24 + 1], // slightly more than 1 day, should not be shown as '1min0s'
['duration', 1000 * 3600 * 24 - 31 * 1000], // 1d - 31s, should be shown as '23h59min'
['duration', 1000 * 3600 * 24 - 30 * 1000], // 1d - 30s, should be shown as '1d', not '0d24h'
['duration', 1000 * 3600 * 24 + 1], // slightly more than 1 day, should not be shown as '1d0h'
['time', 12.3456789],
['seconds', 0],
['seconds', 10],
Expand Down Expand Up @@ -303,11 +306,14 @@ describe('marker schema formatting', function () {
"duration - 13h53min",
"duration - 1d10h",
"duration - 123.46ns",
"duration - 59.499s",
"duration - 1min",
"duration - 1min",
"duration - 13min",
"duration - 59min59s",
"duration - 1h",
"duration - 1h",
"duration - 23h59min",
"duration - 1d",
"duration - 1d",
"time - 12.346ms",
Expand Down
107 changes: 80 additions & 27 deletions src/utils/format-numbers.js
Original file line number Diff line number Diff line change
Expand Up @@ -237,92 +237,145 @@ export function formatMilliseconds(
export function formatSeconds(
time: Milliseconds,
significantDigits: number = 5,
maxFractionalDigits: number = 3
maxFractionalDigits: number = 3,
precision: Milliseconds = Infinity
) {
return (
formatNumber(time / 1000, significantDigits, maxFractionalDigits) + 's'
);
const msPerSecond = 1000;
const timeInSeconds = time / msPerSecond;
let result = '';
if (precision !== 0 && precision < msPerSecond) {
const exponent = Math.floor(Math.log10(precision / msPerSecond));
const digits = Math.max(0, -exponent);
result = timeInSeconds.toFixed(digits);
} else {
result = formatNumber(
timeInSeconds,
significantDigits,
maxFractionalDigits
);
}
return result + 's';
}

export function formatMinutes(
time: Milliseconds,
significantDigits: number = 5,
maxFractionalDigits: number = 2
maxFractionalDigits: number = 2,
precision: Milliseconds = Infinity
) {
const msPerSecond = 1000;
time = Math.round(time / msPerSecond) * msPerSecond;
const msPerMinute = 60 * msPerSecond;
if (precision >= msPerSecond) {
time = Math.round(time / msPerSecond) * msPerSecond;
}
const seconds = time % msPerMinute;
return (
formatNumber((time - seconds) / msPerMinute, significantDigits, 0) +
'min' +
(maxFractionalDigits > 0 && seconds > 0
? formatSeconds(seconds, significantDigits, 0)
(seconds > 0 && (maxFractionalDigits > 0 || precision < msPerMinute)
? formatSeconds(seconds, significantDigits, 0, precision)
: '')
);
}

export function formatHours(
time: Milliseconds,
significantDigits: number = 5,
maxFractionalDigits: number = 1
maxFractionalDigits: number = 1,
precision: Milliseconds = Infinity
) {
const msPerMinute = 60 * 1000;
time = Math.round(time / msPerMinute) * msPerMinute;
if (precision >= msPerMinute) {
time = Math.round(time / msPerMinute) * msPerMinute;
}
const msPerHour = 60 * msPerMinute;
const minutes = time % msPerHour;
return (
formatNumber((time - minutes) / msPerHour, significantDigits, 0) +
'h' +
(maxFractionalDigits > 0 && minutes > 0
? formatMinutes(minutes, significantDigits, 0)
(minutes > 0 && (maxFractionalDigits > 0 || precision < msPerHour)
? formatMinutes(minutes, significantDigits, 0, precision)
: '')
);
}

export function formatDays(
time: Milliseconds,
significantDigits: number = 5,
maxFractionalDigits: number = 1
maxFractionalDigits: number = 1,
precision: Milliseconds = Infinity
) {
const msPerHour = 60 * 60 * 1000;
time = Math.round(time / msPerHour) * msPerHour;
if (precision >= msPerHour) {
time = Math.round(time / msPerHour) * msPerHour;
}
const msPerDay = 24 * msPerHour;
const hours = time % msPerDay;
return (
formatNumber((time - hours) / msPerDay, significantDigits, 0) +
'd' +
(maxFractionalDigits > 0 && hours > 0
? formatHours(hours, significantDigits, 0)
(hours > 0 && (maxFractionalDigits > 0 || precision < msPerDay)
? formatHours(hours, significantDigits, 0, precision)
: '')
);
}

export function formatTimestamp(
time: Milliseconds,
significantDigits: number = 5,
maxFractionalDigits: number = 3
maxFractionalDigits: number = 3,
// precision is the minimum required precision.
precision: Milliseconds = Infinity
) {
if (precision !== Infinity) {
// Round the values to display nicer numbers when the extra precision
// isn't useful. (eg. show 3h52min10s instead of 3h52min14s)
precision = 10 ** Math.floor(Math.log10(precision));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wonder if this part should be done in the caller, so that the precision arguments have the same meaning in all the functions here. What do you think?

Although I'm not 100% sure about this line does. Would we have too much precision without this line?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think doing it in the caller would be more effort once there are multiple callers (I'm hoping the TimelineRuler code will eventually use this code path too). The precision argument still works in the same way in all the functions, thanks to the Math.floor(Math.log10( call in formatSeconds.

I added a comment in the code to explain what behavior these lines implement. Not the most useful part of the patch, unless it was needed to fix an edge case I don't remember.

if (time > precision) {
time = Math.round(time / precision) * precision;
}
}
// Format in the closest base (days, hours, minutes, seconds, milliseconds,
// microseconds, or nanoseconds), to avoid cases where times are displayed
// with too many leading zeroes to be useful.
// The Math.round call is needed to avoid showing values like '0min60s'.
if (Math.round(time / 1000) / 60 >= 1) {
// The if blocks are nested to avoid calling Math.round repeatedly for
// the most common case where the value will be less than 1 minute.
if (Math.round(time / (60 * 1000)) / 60 >= 1) {
if (Math.round(time / (60 * 60 * 1000)) / 24 >= 1) {
return formatDays(time, significantDigits, maxFractionalDigits);
// 59.5s is the smallest value rounded to 1min.
if (time >= 60 * 1000 - (precision === Infinity ? 500 : 0)) {
// The if blocks are nested to avoid repeated tests for the most
// common case where the value will be less than 1 minute.
// 59min59.5s is the smallest value rounded to 1h
if (time >= 60 * 60 * 1000 - (precision === Infinity ? 500 : 0)) {
// 23h59min30s is the smallest value rounded to 1d.
if (
time >=
24 * 60 * 60 * 1000 - (precision === Infinity ? 30 * 1000 : 0)
) {
return formatDays(
time,
significantDigits,
maxFractionalDigits,
precision
);
}
return formatHours(time, significantDigits, maxFractionalDigits);
return formatHours(
time,
significantDigits,
maxFractionalDigits,
precision
);
}
return formatMinutes(time, significantDigits, maxFractionalDigits);
return formatMinutes(
time,
significantDigits,
maxFractionalDigits,
precision
);
}
if (time >= 1000) {
return formatSeconds(
time,
significantDigits,
Number.isInteger(time / 1000) ? 0 : maxFractionalDigits
Number.isInteger(time / 1000) ? 0 : maxFractionalDigits,
precision
);
}
if (time >= 1) {
Expand Down