clients(devtools): use the same desktop throttling as lightrider #10322
Conversation
| * @return {LH.Config.Json} | ||
| */ | ||
| function getDefaultConfigForCategories(categoryIDs) { | ||
| function createConfig(categoryIDs, device) { |
There was a problem hiding this comment.
cc @chruxin maybe a breaking change for you
| // Using a "broadband" connection type | ||
| // Corresponds to "Dense 4G 25th percentile" in https://docs.google.com/document/d/1Ft1Bnq9-t4jK5egLSOc28IL4TvR-Tt0se_1faTA4KTY/edit#heading=h.bb7nfy2x9e5v | ||
| desktopDense4G: { | ||
| rttMs: 40, |
There was a problem hiding this comment.
should we set requestLatencyMs: 0, downloadThroughputKbps: 0, uploadThroughputKbps: 0 for no throttling when --throttling-method=devtools?
There was a problem hiding this comment.
would this be done in just the dt entry's createConfig?
There was a problem hiding this comment.
I assumed we'd set it on this same object like we do for mobile settings
There was a problem hiding this comment.
tbh I've lost the thread here and have no idea how to synthesize what you're saying :')
There was a problem hiding this comment.
but you said when --throttling-method=devtools, so what about for when throttling is set to simulated? That's why I thought this might need to be done in createConfig, behind a check for the throttling method
and is this what we want for lr-desktop-config too?
There was a problem hiding this comment.
Well adding these here is just going to be a noop when --throttling-method=simulate, I was distinguishing that this object should cover both throttling cases like the mobile 4g ones do above.
There was a problem hiding this comment.
+1 to adding =0, imo is more explicit and matches the pattern of all of the ones above.
There was a problem hiding this comment.
did we decide not to do this? my suggestion seems to have a +1 from shane :)
There was a problem hiding this comment.
i forgot to push
|
Either way this PR will have a Chromium counterpart correct? |
Yeah. Even without this change there is. The last change to this file was post-5.7, and it completely changed the interface. #10241 tracks it. |
| // Using a "broadband" connection type | ||
| // Corresponds to "Dense 4G 25th percentile" in https://docs.google.com/document/d/1Ft1Bnq9-t4jK5egLSOc28IL4TvR-Tt0se_1faTA4KTY/edit#heading=h.bb7nfy2x9e5v | ||
| desktopDense4G: { | ||
| rttMs: 40, |
There was a problem hiding this comment.
+1 to adding =0, imo is more explicit and matches the pattern of all of the ones above.
I think so? |
|
@paulirish @patrickhulce I think one of you should sanity check :) |
| // Using a "broadband" connection type | ||
| // Corresponds to "Dense 4G 25th percentile" in https://docs.google.com/document/d/1Ft1Bnq9-t4jK5egLSOc28IL4TvR-Tt0se_1faTA4KTY/edit#heading=h.bb7nfy2x9e5v | ||
| desktopDense4G: { | ||
| rttMs: 40, |
There was a problem hiding this comment.
did we decide not to do this? my suggestion seems to have a +1 from shane :)
Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>
|
GH comment editor somehow added trailing spaces though sorry 😬 |
fixes #10315
I was going to make a shared desktop config and rely on config inheritance, but it turns out that we only support inheriting from the default config :)