core(network-request): simplify recomputeTimesWithResourceTiming #15107
Conversation
| this.responseHeadersEndTime = Math.max(this.responseHeadersEndTime, this.networkRequestTime); | ||
|
|
There was a problem hiding this comment.
when comparing headersReceivedTime with the value of this.responseHeadersEndTime after, for every run I tried (including lantern database) there is no difference. This code originally came from CDT (no explanation there) - best I can reason is that it was to be extra careful that ordering invariants are maintained. I don't think it's ever actually needed, though. Adding a comment here for better readability.
There was a problem hiding this comment.
love it. this readability refactor makes me happy.
also... the DT logic here is still the same today fwiw. https://github.com/ChromeDevTools/devtools-frontend/blob/23cde35228250b831d70b63ab76d05ab4d193129/front_end/core/sdk/NetworkRequest.ts#L737-L754
| // Take networkRequestTime and responseHeadersEndTime from timing data for better accuracy. | ||
| // Timing's requestTime is a baseline in seconds, rest of the numbers there are ticks in millis. | ||
| this.networkRequestTime = timing.requestTime * 1000; | ||
| const headersReceivedTime = this.networkRequestTime + timing.receiveHeadersEnd; | ||
| if (!this.responseHeadersEndTime || this.responseHeadersEndTime < 0) { | ||
| this.responseHeadersEndTime = headersReceivedTime; | ||
| } |
There was a problem hiding this comment.
this literally cannot be true, the one place this is called sets responseHeadersEndTime to a non-zero value first.
| @@ -372,16 +373,22 @@ class NetworkRequest { | |||
| // Don't recompute times if the data is invalid. RequestTime should always be a thread timestamp. | |||
| // If we don't have receiveHeadersEnd, we really don't have more accurate data. | |||
| if (timing.requestTime === 0 || timing.receiveHeadersEnd === -1) return; | |||
|
|
|||
| // Take networkRequestTime and responseHeadersEndTime from timing data for better accuracy. | |||
There was a problem hiding this comment.
| // Take networkRequestTime and responseHeadersEndTime from timing data for better accuracy. | |
| // networkRequestTime and responseHeadersEndTime are canonically defined in resource timing. Up till now, responseHeadersEndTime has a bogus fallback that was set in _onResponse(). |
There was a problem hiding this comment.
lmk if the new comment is still lacking for u
| this.responseHeadersEndTime = Math.max(this.responseHeadersEndTime, this.networkRequestTime); | ||
|
|
There was a problem hiding this comment.
love it. this readability refactor makes me happy.
also... the DT logic here is still the same today fwiw. https://github.com/ChromeDevTools/devtools-frontend/blob/23cde35228250b831d70b63ab76d05ab4d193129/front_end/core/sdk/NetworkRequest.ts#L737-L754
| @@ -17,7 +17,7 @@ fi | |||
|
|
|||
| printf "Determined the following files have been touched:\n\n$CHANGED_FILES\n\n" | |||
|
|
|||
| if ! echo $CHANGED_FILES | grep -E 'dependency-graph|metrics|lantern|predictive-perf|network-recorder' > /dev/null; then | |||
| if ! echo $CHANGED_FILES | grep -E 'dependency-graph|metrics|lantern|predictive-perf|network-recorder|network-request' > /dev/null; then | |||
There was a problem hiding this comment.
How valuable is this check? Are we worried about the extra CI compute time it would take to run this on every PR?
I'm worried that this list is incomplete, or that we may forget to update this list when adding new lantern relevant files to the repo.
There was a problem hiding this comment.
smokes def. take most of the time of a run, and this is only ~2m. mostly worried about the constant downloading of this thing and how that could upset cloud storage?
connorjclark
changed the title
Adds a comment and removes a conditional that is impossible to be true.