core(network): handle LR transferSize #5895
Conversation
|
|
||
| declare module NodeJS { | ||
| interface Global { | ||
| isLightRider?: boolean; |
There was a problem hiding this comment.
@brendankenny on a scale of 1-5, how offensive is this to you?
| if (!originalContentLength) return; | ||
|
|
||
| // Transfer size is the original content length + length of headers | ||
| const contentBytes = parseFloat(originalContentLength.value); |
| */ | ||
| _updateTransferSizeForLightRiderIfNecessary() { | ||
| // Bail if we're not in LightRider, this only applies there. | ||
| if (!global.isLightRider) return; |
There was a problem hiding this comment.
I figure this is the best global reference we got.. better than self? WFM
There was a problem hiding this comment.
yeah was just using what we decided to go with in web-inspector.js last time
| @@ -95,6 +95,9 @@ async function runLighthouseInExtension(flags, categoryIDs) { | |||
| * @return {Promise<string|Array<string>|void>} | |||
| */ | |||
| async function runLighthouseInLR(connection, url, flags, {lrDevice, categoryIDs, logAssets}) { | |||
| // Certain fixes need to kick-in under LR, see https://github.com/GoogleChrome/lighthouse/issues/5839 | |||
There was a problem hiding this comment.
One if the origin RTT estimates only applies in LR. Do you want to re use this there? Fine if you don't.
There was a problem hiding this comment.
Nah that's ok that fallback is a lot more sound than this one :)
There was a problem hiding this comment.
on a scale of 1-5, how offensive is this to you?
ha, well I medium hate it. Not sure of the numeric value of that.
I'm pretty ok with it since it's pretty much constant from the start of the run and so doesn't cause weird non-local effects that are difficult to trace through the codebase. Buuuuuut, looking at #4972 again, if we're going to be piping the channel around for that anyways (through process.env.LH_CHANNEL or in flags or whatever), I wonder if we should just piggy back on that.
It is a pain to get settings down into NetworkRequest, though. Seems like the NetworkRecords computed artifact would need a whole computed artifact re-architecture, so maybe something global is inevitable...
sounds good enough to me :)
Yeah this is what I was thinking as well, I did not like the mental chain of what would need to accept |
Summary
In LR's case we don't have real transfer size, we just have the header of original content length. I went with the
isLightRiderglobal var for a few reasons.sizes now look good with the traces/devtoolslog paul provided
Related Issues/PRs
fixes #5839