Rename "round trip time" to "latency" in network throttling

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Developer Tools: Responsive Design Mode
P3
normal
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: jryans, Assigned: oliver.scheiwiller, Mentored)

Tracking

(Blocks: 1 bug, {good-first-bug})

49 Branch
Firefox 53
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 months ago
The network throttling code uses the name "round trip time" in several places, but I think "latency" would be more accurate, since the value is used as a single delay before data starts to stream in.

(To me, RTT is the sum of propagation delay in both directions, which is not what the value is used for here.)
(Reporter)

Comment 1

7 months ago
I believe the files to change here include:

devtools/client/netmonitor/har/test/browser_net_har_throttle_upload.js
devtools/client/netmonitor/test/browser_net_throttle.js
devtools/server/actors/emulation.js
devtools/shared/webconsole/test/unit/test_throttle.js
devtools/shared/webconsole/throttle.js

There should be no change in functionality, this should just be a rename within the code base.

To get started with the DevTools code base, take a look at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build and https://wiki.mozilla.org/DevTools/Hacking.

Please set needinfo? to mentor (my email) if you have any questions.
Mentor: jryans@gmail.com
Keywords: good-first-bug
(Reporter)

Updated

7 months ago
Priority: -- → P3
(Assignee)

Comment 2

7 months ago
Created attachment 8811150 [details] [diff] [review]
Patch for Bug 1317745 - Rename "round trip time" to "latency" in network throttling
Attachment #8811150 - Flags: review?(jryans)
(Reporter)

Comment 3

7 months ago
Comment on attachment 8811150 [details] [diff] [review]
Patch for Bug 1317745 - Rename "round trip time" to "latency" in network throttling

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

I think this look reasonable overall, thanks for working on it!

I found one small style nit below, and also please update the commit message[1] to add "r=jryans" at the end to note the reviewer name.

Once you fixed those things, attach an updated patch here, request review again, and mark the old attachment obsolete (you can do this from the screen where you attach the new one).

Assuming the next patch looks good, I'll push it to our CI infrastructure for testing.

[1]: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions

::: devtools/shared/webconsole/throttle.js
@@ +230,3 @@
>   */
>  function NetworkThrottleQueue(meanBPS, maxBPS,
> +                              latencyMean, latencyMax) {

Nit: I think all these args will fit on one line now...?  DevTools uses a 90 char line length.
Attachment #8811150 - Flags: review?(jryans)
(Reporter)

Updated

7 months ago
Assignee: nobody → oliver.scheiwiller
Status: NEW → ASSIGNED
(Assignee)

Comment 4

7 months ago
Created attachment 8812073 [details] [diff] [review]
Updated patch for Bug1317745 - Rename "round trip time" to "latency" in network throttling
Attachment #8811150 - Attachment is obsolete: true
Attachment #8812073 - Flags: review?(jryans)
(Reporter)

Comment 5

7 months ago
Comment on attachment 8812073 [details] [diff] [review]
Updated patch for Bug1317745 - Rename "round trip time" to "latency" in network throttling

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

Great, this version looks good to me!  (Sorry for the delay, I was out of the office for a few days.)

I'll push this to our CI infrastructure for testing.
Attachment #8812073 - Flags: review?(jryans) → review+
(Reporter)

Comment 6

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ac02ce7a5949777b238120f723f973cc269a041
(Reporter)

Comment 7

7 months ago
Try run looks good, I'll mark this for check in.  One of our sheriffs will land the patch and resolve the bug once it reaches mozilla-central.

Thanks again for helping with this!  Please check out other DevTools bugs on http://firefox-dev.tools or elsewhere.
Keywords: checkin-needed

Comment 8

7 months ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c3caa5cf8a6
Rename 'round trip time' to 'latency' in network throttling. r=jryans
Keywords: checkin-needed

Comment 9

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8c3caa5cf8a6
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.