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

Cap LCP load duration to LCP time #527

Merged
merged 6 commits into from
Sep 27, 2024
Merged

Cap LCP load duration to LCP time #527

merged 6 commits into from
Sep 27, 2024

Conversation

tunetheweb
Copy link
Member

Fixes #509

Videos can have LCP time before finished download so the resourceLoadDuration should be capped to LCP time.

I can't see an easy way to add a test case for this, but think the change is fairly innocuous.

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

I can't see an easy way to add a test case for this, but think the change is fairly innocuous.

How hard would it be to add a video LCP test?

src/attribution/onLCP.ts Outdated Show resolved Hide resolved
src/attribution/onLCP.ts Outdated Show resolved Hide resolved
@tunetheweb
Copy link
Member Author

I can't see an easy way to add a test case for this, but think the change is fairly innocuous.

How hard would it be to add a video LCP test?

Well it's not just that. We'd need to load a video, and draw the LCP from the first frame, but ensure the video is still downloading after that LCP. That sounds incredibly racy. Plus the implementation in Chrome seems to be broken at the moment which Ian is looking into.

Plus need to consider other browsers (e.g. Firefox), which don't support video yet (I don't think) so might be OK, but another complication...

@philipwalton
Copy link
Member

Well it's not just that. We'd need to load a video, and draw the LCP from the first frame, but ensure the video is still downloading after that LCP. That sounds incredibly racy.

I'm not sure if it's possible in Express, but it seems like we should be able to create a stream object that reads the video file on disk and adds an arbitrary setTimeout half way through to simulate this case without having to rely on the racy-ness of the network.

@tunetheweb
Copy link
Member Author

I'm not sure if it's possible in Express, but it seems like we should be able to create a stream object that reads the video file on disk and adds an arbitrary setTimeout half way through to simulate this case without having to rely on the racy-ness of the network.

Still depends on Chrome working as it's supposed to though. So point is kinda moot for now.

@tunetheweb
Copy link
Member Author

Raised this bug for the Chrome issue: https://issues.chromium.org/issues/364860066

Once that's resolved (and that version of Chrome feeds through to our test suite) we should see how to add a test, but until then I still think we should merge this change in preparatation.

src/attribution/onLCP.ts Outdated Show resolved Hide resolved
src/attribution/onLCP.ts Show resolved Hide resolved
@brendankenny
Copy link
Member

Should this wait until we have a clear picture of what exactly ships (and when) in Chrome?

@tunetheweb
Copy link
Member Author

Should this wait until we have a clear picture of what exactly ships (and when) in Chrome?

I don't see any reason NOT to cap it at the metric value anyway. Especially as I know of at least one reason (that's supposed to have shipped already).

src/attribution/onLCP.ts Show resolved Hide resolved
@philipwalton
Copy link
Member

Agreed. This seems low risk and may help avoid future bugs.

@philipwalton philipwalton merged commit f18c7ee into v5 Sep 27, 2024
6 checks passed
@philipwalton philipwalton deleted the cap-lcp-load-duration branch September 27, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants