core(trace): compute trace for main frame and any child frames#11760
Merged
Conversation
connorjclark
requested changes
Dec 4, 2020
connorjclark
approved these changes
Dec 21, 2020
connorjclark
left a comment
Collaborator
There was a problem hiding this comment.
LGTM.
An optional refactor idea I leave to you (and @patrickhulce to weigh in) to decide on.
- modify
computeValidLCPAllFramesto also return the lcp of each frame - call
computeValidLCPAllFramesjust once incomputeTraceOfTab - either pass the main frame result to
computeKeyTimingsForFrame; or circumvent that fn entirely and set the appropriate return values directly in the return statement ofcomputeTraceOfTab
Contributor
Author
I like explicitly computing the main frame LCP separately in I'll try to refractor this in the next PR for FCP since we would probably want a similar change for that event. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Follow up to #11701 and #11713.
@paulirish recognized how loading two pages at once will lead to issues when calculating LCP for all frames #11701 (comment)
This PR makes some substantial changes to the trace processor which allows us to calculate the trace for the main frame and any child frames:
FrameCommittedInBrowserevents.frameTreeEvents.largestContentfulPaint::*events inframeTreeEventsto calculate LCP from all frames. The previous behavior introduced in core(metrics): support LCP in all frames for devtools throttling #11701 looked forNavStartToLargestContentfulPaint::*::AllFrames::UKMevents in the entire trace.frameTreeEventsinstead of the entire trace to calculate CLS from all frames.FrameCommittedInBrowserevents. For future works, this trace can test FCP from all frames, this is so we won't need to update it again when that gets done.