Show the hovered time in the ruler at the top of the timeline.

[fqueze]

[codecov]

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (3e8d88f) 88.33% compared to head (128bd24) 88.32%.
Report is 8 commits behind head on main.

❗ Current head 128bd24 differs from pull request most recent head 6205fb5. Consider uploading reports for the commit 6205fb5 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4748      +/-   ##
==========================================
- Coverage   88.33%   88.32%   -0.02%     
==========================================
  Files         300      300              
  Lines       26768    26794      +26     
  Branches     7220     7237      +17     
==========================================
+ Hits        23646    23666      +20     
- Misses       2910     2916       +6     
  Partials      212      212              

Files	Coverage Δ
src/components/timeline/Selection.js	79.54% <100.00%> (+0.47%)	⬆️
src/profile-logic/committed-ranges.js	84.84% <100.00%> (+0.23%)	⬆️
src/utils/format-numbers.js	85.02% <79.54%> (-1.87%)	⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[julienw]

This looks pretty good to me, however I think that getFormattedTimeLength doesn't do the right thing in this case. Please have a look at my suggestion instead :-)

[julienw]

Can you please explain why this change in a comment?

[fqueze]

Do you want a comment in the code or is a comment here good enough?

The reason was that when moving the mouse around over the profiler page while the profiler was still loading, this code was triggered, with width == 0, causing the mouseTimePosition to become Infinity, and my new code to explode attempting to number format NaN.

I think a cleaner fix would be finding a way to register the mousemove handler later when the profile has finished loading, but returning early when width === 0 seemed good enough to avoid the crash.

[julienw]

Would it be better to do an early return for width==0? It would be good to have a proper comment in the code too indeed :-)

[julienw]

I believe it should use a similar logic for the precision than the ruler, because otherwise it's not useful when zoomed in a lot (except at the start of the profile).
The logic is in timeline/Ruler.js, but here is what is I believe a good enough approximation:

const range = committedRange.end - committedRange.start;
const exponent = Math.floor(Math.log10(range));
const digits = Math.max(0, -(exponent - 4))
const timeString = (mouseTimePosition - zeroAt).toFixed(digits);

I haven't tried it though, tell me how that looks.

[fqueze]

I actually care more about showing something useful for long profiles (ie. not hundreads or thousands of seconds, but something in minutes and seconds) than about the high precision when zoomed into extremely short intervals.
Of course having both would be best, but that might require a lot more, like an alternative version of formatNumber(or adding an extra parameter to it) to specify the required smallest unit to display.

The numbers displayed in the ruler are currently difficult to read for long profiles. That's something I would like to improve someday. Maybe I could fix the numbers displayed in the ruler and polish the number in this new overlay at the same time?

A short term alternative if you really care about short intervals could be to switch to the implementation you suggested when the commited range lasts less than a few seconds. Not sure if that would be better or more confusing.

[julienw]

I believe you could do:

const range = committedRange.end - committedRange.start;
const exponent = Math.floor(Math.log10(range));
const digits = Math.max(0, -(exponent - 4))
const timeString =
  digits > 0
  ? ((mouseTimePosition - zeroAt)/1000).toFixed(digits) + "s"
  : getFormattedTimeLength(mouseTimePosition - zeroAt);
}

[fqueze]

I thought about it some more, and I think the correct behavior is to ensure that whenever we move the mouse by 1px, the displayed time changes. So we need to adjust the precision of the displayed timestamp based on how much time is represented by 1px. I updated the PR to do that.

Here's a long (> 1 day) profile that I used for testing: https://share.firefox.dev/46mER8G

[julienw]

Would it be better to do an early return for width==0? It would be good to have a proper comment in the code too indeed :-)

[julienw]

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?

[fqueze]

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.