Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: fix throttling rate comment and tweak comcast bandwidth #7374

Merged
merged 2 commits into from
Mar 5, 2019

Conversation

mattzeunert
Copy link
Contributor

Summary

Throttling rate is in kilobits not kilobytes.

LH uses a bandwidth of 1638kbps/1.6mpbs by default, so we should recommend using that with comcast too.

@paulirish
Copy link
Member

paulirish commented Mar 5, 2019

the throttling.md change works for me.

But I'm not sure about the kilobytes => kilobits change.

EDIT: nevermind. Lantern seems to consistently treat "throughput" as bits per second.

@paulirish
Copy link
Member

paulirish commented Mar 5, 2019

Took another look and Lantern is good.

Also confirmed that DevTools's emulateNetworkConditions speaks in bytes per second.
That leads me to wonder why we never convert our configuration value... My best guess is it's somehow wrapped up in the DEVTOOLS_THROUGHPUT_ADJUSTMENT_FACTOR but that seems very odd... @patrickhulce plz tell me i'm being really dumb here. :)

@paulirish paulirish removed their request for review March 5, 2019 00:58
@patrickhulce
Copy link
Collaborator

patrickhulce commented Mar 5, 2019

We do convert our numbers before sending to DevTools, so I think we're all good here.

// DevTools expects throughput in bytes per second rather than kbps
conditions.downloadThroughput = Math.floor(conditions.downloadThroughput * 1024 / 8);
conditions.uploadThroughput = Math.floor(conditions.uploadThroughput * 1024 / 8);
return driver.sendCommand('Network.emulateNetworkConditions', conditions);

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM!

Nice catch @mattzeunert yeah the lowercase b in Kbps was meant to be the signal for kilobits per second, not sure how we messed that doc string up 😆

@paulirish paulirish merged commit 9ba7199 into master Mar 5, 2019
@paulirish paulirish deleted the throttling-docs-fix branch March 5, 2019 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants