Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(trace): compute trace for main frame and any child frames #11760

Merged
merged 7 commits into from
Dec 21, 2020

Conversation

adamraine
Copy link
Member

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:

  1. Determine the root ancestor frame for each frame using FrameCommittedInBrowser events.
  2. Trace events belonging to the main frame tree for the page are put into frameTreeEvents.
  3. Use largestContentfulPaint::* events in frameTreeEvents to calculate LCP from all frames. The previous behavior introduced in core(metrics): support LCP in all frames for devtools throttling #11701 looked for NavStartToLargestContentfulPaint::*::AllFrames::UKM events in the entire trace.
  4. Use frameTreeEvents instead of the entire trace to calculate CLS from all frames.
  5. Update the minified test trace to include FrameCommittedInBrowser events. 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.

@adamraine adamraine requested a review from a team as a code owner December 3, 2020 23:55
@adamraine adamraine requested review from connorjclark and removed request for a team December 3, 2020 23:55
@google-cla google-cla bot added the cla: yes label Dec 3, 2020
lighthouse-core/lib/tracehouse/trace-processor.js Outdated Show resolved Hide resolved
lighthouse-core/lib/tracehouse/trace-processor.js Outdated Show resolved Hide resolved
lighthouse-core/lib/tracehouse/trace-processor.js Outdated Show resolved Hide resolved
lighthouse-core/lib/tracehouse/trace-processor.js Outdated Show resolved Hide resolved
types/externs.d.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

An optional refactor idea I leave to you (and @patrickhulce to weigh in) to decide on.

  • modify computeValidLCPAllFrames to also return the lcp of each frame
  • call computeValidLCPAllFrames just once in computeTraceOfTab
  • either pass the main frame result to computeKeyTimingsForFrame; or circumvent that fn entirely and set the appropriate return values directly in the return statement of computeTraceOfTab

lighthouse-core/lib/tracehouse/trace-processor.js Outdated Show resolved Hide resolved
@adamraine
Copy link
Member Author

  • modify computeValidLCPAllFrames to also return the lcp of each frame
  • call computeValidLCPAllFrames just once in computeTraceOfTab
  • either pass the main frame result to computeKeyTimingsForFrame; or circumvent that fn entirely and set the appropriate return values directly in the return statement of computeTraceOfTab

I like explicitly computing the main frame LCP separately in computeKeyTimingsForFrame and passing the main frame result as a parameter just to return it seems weird.

I'll try to refractor this in the next PR for FCP since we would probably want a similar change for that event.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants