core(network-request): switch to improved timing names #14721
Conversation
brendankenny
requested review from
connorjclark
and removed request for
a team
| @@ -41,42 +41,42 @@ class CriticalRequestChains extends Audit { | |||
| }; | |||
| } | |||
|
|
|||
| /** @typedef {{depth: number, id: string, chainDuration: number, chainTransferSize: number, node: LH.Audit.Details.SimpleCriticalRequestNode[string]}} CrcNodeInfo */ | |||
| /** @typedef {{depth: number, id: string, chainDuration: number, chainTransferSize: number, node: LH.Artifacts.CriticalRequestNode[string]}} CrcNodeInfo */ | |||
There was a problem hiding this comment.
This file has the only real changes. This was actually always LH.Artifacts.CriticalRequestNode because the input was the (computed) artifact, with node.request being an actual NetworkRequest, but previous to this PR that was actually structurally compatible with the simplified LH.Audit.Details.SimpleCriticalRequestNode:
lighthouse/types/lhr/audit-details.d.ts
Lines 32 to 42 in a2977ff
so the two types could be used interchangeably.
Since we don't want to change LH.Audit.Details.SimpleCriticalRequestNode, but node.request now has networkRequestTime, etc on it, this file needs to now actually differentiate between the two, and there needs to be a copy step from the new property names to the old property names on the audit details.
Nothing actually changes in the output (see sample_v2.json for an example).
| @@ -199,7 +199,7 @@ class CriticalRequestChains extends Audit { | |||
| walk(initialNavChildren, 0); | |||
| } | |||
|
|
|||
| const longestChain = CriticalRequestChains._getLongestChain(flattenedChains); | |||
| const longestChain = CriticalRequestChains._getLongestChain(chains); | |||
There was a problem hiding this comment.
AFAICT this never made sense to run on flattenedChains, since it recomputes the durations anyways, but it previously worked either way. Now it just needs to drop the * 1000 since request times are in milliseconds after #14567
| const timeSinceMainEnd = Math.max(0, record.networkRequestTime - mainResource.networkEndTime); | ||
| return timeSinceMainEnd < PRECONNECT_SOCKET_MAX_IDLE_IN_MS; |
There was a problem hiding this comment.
the only other real code change, because it was kind of hard to read split over two lines. An intermediate variable seemed fine.
Alternative to #14574 doing only renames, no functional changes:
s/startTime/networkRequestTime/gs/endTime/networkEndTime/gs/responseReceivedTime/responseHeadersEndTime/gThis avoids the need for getters in
network-requestand the support for both new and old property names innetwork-records-to-devtools-log.It also doesn't add
rendererEndTime. That could still be done in a follow up, but since it wouldn't do anything different thannetworkEndTimefor now, maybe it's better as an issue?Also required:
I don't think we want to change the audit details because that would require a
prepareReportResultsection to convert existing CRC details, and it seems probable we'll come up with new critical path audits and ditch the old CRC stuff before too long :)Need to commit with
Co-authored-by: Connor Clark <cjamcl@gmail.com>since @connorjclark did all the very lengthy work dealing with all our bikeshedding of the names :)