core(redirects): use requestId instead of URL to find requests #14838
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brendankenny
requested review from
connorjclark
and removed request for
a team
brendankenny
commented
Feb 28, 2023
| * GET /thirdRedirect => 302 /mainDocumentUrl | ||
| * GET /mainDocumentUrl => 200 /mainDocumentUrl |
There was a problem hiding this comment.
not that important, but this didn't quite make sense (there had to be one last redirect)
Comment on lines
-76
to
-77
| // Use the main resource as a backup if we didn't find any modern navigationStart events | ||
| return (mainResource.redirects || []).concat(mainResource); |
There was a problem hiding this comment.
this is for super ancient traces (like m60, maybe), and there are no tests hitting this, so no need to keep it around.
| @@ -96,10 +98,10 @@ class Redirects extends Audit { | |||
| const metricResult = await LanternInteractive.request(metricComputationData, context); | |||
|
|
|||
| /** @type {Map<string, LH.Gatherer.Simulation.NodeTiming>} */ | |||
| const nodeTimingsByUrl = new Map(); | |||
There was a problem hiding this comment.
similar to the navigationId change above, this gets confused if there are multiple resources per URL. The graph has the full records, so this can match on the actual unique ID.
| @@ -709,7 +709,7 @@ declare module Artifacts { | |||
| timestamps: TraceTimes; | |||
| /** The relative times from timeOrigin to key events, in milliseconds. */ | |||
| timings: TraceTimes; | |||
| /** The subset of trace events from the page's process, sorted by timestamp. */ | |||
| /** The subset of trace events from the main frame's process, sorted by timestamp. Due to cross-origin navigations, the main frame may have multiple processes, so events may be from more than one process. */ | |||
There was a problem hiding this comment.
Tried to clarify what "processEvents" means after @paulirish's change in #14287
adamraine
approved these changes
Feb 28, 2023
brendankenny
force-pushed
the
redirects-fix
branch
from
b91c8a5 to
6c563ed
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pages that do a js redirect to themselves (e.g. found pages doing some janky A/B testing that would reload after doing a sync XHR) were getting no feedback from the
redirectsaudit because the audit was matching requests to navStarts by URL. Repeated navStarts to the same URL would always resolve to the first network request, so the audit would say the redirects took 0ms.Instead, this PR looks at the navStart
navigationId, which matches network requests'requestIdfor navigation requests.Also adds redirect assertions to all the
redirect-*smoke tests, because we don't have great trace fixture coverage of redirect cases, but we do have great smoke test coverage of them.