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.

Sign up for GitHub

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

Jump to bottom

strip out references to rustdoc's custom favicon #330

Closed
wants to merge 1 commit into from
Closed

strip out references to rustdoc's custom favicon #330

wants to merge 1 commit into from

Conversation

QuietMisdreavus
Copy link
Member

Starting in rust-lang/rust#57552, rustdoc started packaging in a default column logo and favicon. We started hosting the logo in #290, but ignored the favicon, since docs.rs has its own. However, since rustdoc includes a <link rel="shortcut icon"> tag in the <head>, this overrides the behavior of using /favicon.ico, and creates a lot of extra 404s as people try to access a favicon that isn't there. This PR strips out that favicon reference, which puts a favicon back into documentation pages again.

(Note that this will also strip out custom favicons provided by crates, like the one used by Rocket. If desired, i could add in a different check that would only filter out ones rooted locally (path either begins with / or ..) which would allow custom favicons set by crates to still work.)

@GuillaumeGomez
Copy link
Member

Alongside this, wouldn't it be simpler to generate crates with your own favicon?

@QuietMisdreavus
Copy link
Member Author

We don't currently have the option for the favicon URL to be set via a CLI argument. If we did that, then we could replace this PR with one that passed --html-favicon-url /favicon.ico when generating docs, instead. However, in that case we would still have the current docs that wouldn't have been generated with that option, which would still 404 when trying to load the favicon.

@QuietMisdreavus
Copy link
Member Author

Another option is to change the web server to redirect all *.ico routes to the static favicon.ico path. That would also mean not having to change upstream rustdoc, nor would it require this PR. (Finding ways to not need this PR is helpful because i'm afraid it will increase resource usage for doc page loads. >_>)

@GuillaumeGomez
Copy link
Member

Well, that last solution sounds good! :) Meanwhile, I'll add an option to allow to pass a favicon through CLI if you want?

@QuietMisdreavus QuietMisdreavus mentioned this pull request Apr 29, 2019
Merged
@QuietMisdreavus
Copy link
Member Author

I managed to get the *.ico redirect working, though maybe not super-cleanly. It's posted as #340.

@QuietMisdreavus QuietMisdreavus deleted the no-more-favicons branch May 3, 2019 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@QuietMisdreavus @GuillaumeGomez