868045 - At some threshold, we should convert MS to Seconds in the Waterfall

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[Rob Campbell [:rc] (:robcee)]

Attachment: screen shot showing large ms values

After a few thousand ms, we should start labeling the timeline in seconds. Maybe at 10,000ms?

Comment 1

[Geoff Lankow (:darktrojan)]

I have a rough patch for this but I'll finish bug 874363 before working on it more. 10000ms seems high, it looks weird having e.g. 8192ms on the timeline.

Comment 2

[Victor Porof [:vporof][:vp]]

Thank you for taking this, Geoff!

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: v1

I've done a few things here:
* made the smallest possible increment 125ms (1/8th of a second) instead of 80ms
* switched to displaying seconds if the smallest increment is 1000ms or larger
* tweaked the algorithm for displaying tick marks and the background

I haven't updated the test yet.

Comment 4

[Geoff Lankow (:darktrojan)]

Try was quiet so I've got it to make builds with this patch:
https://tbpl.mozilla.org/?tree=Try&rev=8070d44b63ff

Comment 5

[Victor Porof [:vporof][:vp]]

Comment on attachment 758925 [details] [diff] [review]
v1

Review of attachment 758925 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! This looks good, apart from a few (maybe) unnecessary changes. Let me know if I can help with anything for the test.

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +12,4 @@
>  const REQUESTS_REFRESH_RATE = 50; // ms
>  const REQUESTS_HEADERS_SAFE_BOUNDS = 30; // px
>  const REQUESTS_WATERFALL_SAFE_BOUNDS = 90; // px
> +const REQUESTS_WATERFALL_HEADER_TICKS_MULTIPLE = 125; // ms

Why 125? You were pretty much guaranteed to never get 5, it was just the lowest multiple possible. In practice, it just makes sure that the bars intervals are spaced by numbers ending in 5 or 0.

Please revert this to 5 if you don't have a good reason for changing it.

@@ +16,2 @@
>  const REQUESTS_WATERFALL_HEADER_TICKS_SPACING_MIN = 60; // px
> +const REQUESTS_WATERFALL_BACKGROUND_TICKS_MULTIPLE = 125; // ms

Ditto.

@@ +18,2 @@
>  const REQUESTS_WATERFALL_BACKGROUND_TICKS_SCALES = 3;
> +const REQUESTS_WATERFALL_BACKGROUND_TICKS_SPACING_MIN = 15; // px

Ditto.

@@ +944,3 @@
>        }
> +      timingStep <<= 1;
> +    }

I also don't entirely understand why you reversed the while logic, using 'break' instead of 'continue'. If I'm reading this correctly, it does the exact same thing :)

Don't hate me for saying this, but please revert it if you don't have a good reason for this. Maybe readability was on your mind? That's pretty subjective I'd say, and both implementations can be argued upon. Hell, you could even turn this in a for loop if you wanted to :D

@@ +1006,2 @@
>  
> +    while (true) {

Ditto :)

Comment 6

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 758925 [details] [diff] [review]
v1

Review of attachment 758925 [details] [diff] [review]:
-----------------------------------------------------------------

what victor said! canceling feedback.

Comment 7

[Victor Porof [:vporof][:vp]]

Sad to see you unassign yourself Geoff. Would it be ok if I carried on your work?

Comment 8

[Geoff Lankow (:darktrojan)]

Go for it. I haven't the time.

Comment 10

[Victor Porof [:vporof][:vp]]

Attachment: v2

Comment 11

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8355859 [details] [diff] [review]
v2

Review of attachment 8355859 [details] [diff] [review]:
-----------------------------------------------------------------

looks good. I'll try this out locally, but expect it'll Just Work.

Thanks for starting this, Geoff!

Comment 12

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/05d481f3c49c

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05d481f3c49c