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

Gather LargestContentfulPaint in Firefox. #2018

Merged
merged 3 commits into from
Nov 13, 2023

Conversation

gmierz
Copy link
Collaborator

@gmierz gmierz commented Nov 13, 2023

This patch adds the ability to gather LargestContentfulPaint with Firefox. It's gathered into a firefoxWebVitals entry similar to what is done with Chrome-based browsers.

@gmierz gmierz requested a review from soulgalore November 13, 2023 12:59
@gmierz
Copy link
Collaborator Author

gmierz commented Nov 13, 2023

Hi @soulgalore! Let me know what you think about this approach. I was thinking it might be best to keep these in a single entry (maybe called webVitals), but it looks like the googleWebVitals is used quite a bit everywhere so I didn't change anything there.

@soulgalore
Copy link
Member

soulgalore commented Nov 13, 2023

Hi @gmierz let me get back on that later today or early tomorrow. I think the metrics should work and not need to be added for Firefox, but let me check. I tried the LCP in Firefox nightly sometime ago and it worked fine. I also compared it with the largest contentful paint for the video and it most cases it correlated.

Let me just explain why it's named Google Web Vitals: So ... Google comes up with a series of metrics they say are vital to the web. I don't think they are in the position to define what's vital all across the web. What's vital depends on the web site and the user. But I guess these metrics are vital for getting hight score in Google SEO, so that's why its named Google Web Vitals.

@soulgalore
Copy link
Member

@gmierz what was the idea with this PR? :) I see that we don't add Firefox metrics to the GoogleWebVitals field (missing FCP and LCP), maybe that is the fix that needs to be done? The standalone metric is collected by default, so if I run:
browsertime -b firefox --firefox.nightly https://www.sitespeed.io the largest contentful paint is there.

@gmierzwinski
Copy link

Ah ok, that explains it, thanks! :)

I wasn't sure how we should handle the metric from Firefox. I'll change this to add it to googleWebVitals instead.

@gmierz
Copy link
Collaborator Author

gmierz commented Nov 13, 2023

Ok, thanks for the review @soulgalore, I've removed the firefoxWebVitals entry and added it to the googleWebVitals instead.

@soulgalore soulgalore merged commit 62de4fc into sitespeedio:main Nov 13, 2023
12 checks passed
@soulgalore
Copy link
Member

Thank you @gmierz !

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