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

Should budgets include requests to favicons? #9874

Closed
connorjclark opened this issue Oct 22, 2019 · 10 comments · Fixed by #10190
Closed

Should budgets include requests to favicons? #9874

connorjclark opened this issue Oct 22, 2019 · 10 comments · Fixed by #10190
Labels

Comments

@connorjclark
Copy link
Collaborator

Because they currently do.

I think it'd make sense to exclude favicons from our budget calculations. Has this been discussed?

Note, favicons are not supported in LR, so there is a discrepancy there.

@patrickhulce
Copy link
Collaborator

seems like this should be taken up with the budget folks rather than here ;)

we reuse the definitions over in https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/computed/resource-summary.js#L38-L67 so whatever gets set there will be picked up here 👍

@connorjclark connorjclark transferred this issue from GoogleChrome/lighthouse-ci Oct 22, 2019
@connorjclark
Copy link
Collaborator Author

I did not mean to file this in ci :)

@connorjclark
Copy link
Collaborator Author

@khempenius what are your thoughts on this?

@khempenius
Copy link
Collaborator

IMO they should. Although they're tiny, the page still had to load them.

@connorjclark
Copy link
Collaborator Author

AFIU the request for the favicon doesn't block the renderer in any way, and the browser caches it pretty well for navigations within the same frame, so it only gets requested once.

Those are somewhat-decent justifications IMO, but the real issue I'm thinking of is the constraint in PSI/web.dev where we don't see favicon requests, so budget results differ with other channels.

@khempenius
Copy link
Collaborator

Ah, in which case I'd probably be in favor of not including them in budgets. From the sounds of it, PSI/web.dev not seeing favicons is a pretty concrete constraint and/or a pain to fix, in which case, I think it's more important that all LH versions generate consistent results, than it is that favicons get counted towards budgets.

@haideralipunjabi
Copy link

I see that the budget is ignoring the .ico version of favicon. Is there a reason for it to include the .png format ones?

@patrickhulce
Copy link
Collaborator

A reason is that favicon.ico is requested whether the page wanted it to happen or not whereas favicon.png should only happen when explicitly referenced from the page. I'm not sure that's a great reason, but it makes sure users who don't serve a favicon aren't being punished in budgets for Chrome-behavior.

I'm not sure if headless will request any <link rel="icon"> declaration though, I'd assume it doesn't. If so, our original rationale would suggest ignoring them even if it's much more problematic (could now be named anything, requires LinkElements to be able to filter out, etc)

@connorjclark
Copy link
Collaborator Author

Just checking .ico was an oversight.

@connorjclark
Copy link
Collaborator Author

we got rid of budgets #15950

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