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. |
exterkamp
left a comment
There was a problem hiding this comment.
This looks fine to me. Looks like it has been hashed out by others. I'm fine to land this once the last bit of nit's are worked out.
Is this ready to go at this point?
| // 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 :) |
patrickhulce
left a comment
There was a problem hiding this comment.
if the extra throttling properties issue has been resolved another way I'm unaware of then LGTM :)
| // 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 :)