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

Serve .ico as image/vnd.microsoft.icon #11711

Open
wants to merge 1 commit into
base: jetty-12.0.x
Choose a base branch
from

Conversation

maffe
Copy link

@maffe maffe commented Apr 27, 2024

@sbordet
Copy link
Contributor

sbordet commented Apr 29, 2024

@maffe you need to sign the Eclipse ECA in order to contribute.
Seee https://github.com/jetty/jetty.project/blob/jetty-12.0.x/CONTRIBUTING.md.

@sbordet sbordet self-requested a review April 29, 2024 21:23
@joakime
Copy link
Contributor

joakime commented Apr 30, 2024

Be careful here.

The extension *.ico can be either the Microsoft format (what this PR is attempting to change to) or a PNG or a JPG (what the image/x-icon) supports.

See also https://stackoverflow.com/questions/13827325/correct-mime-type-for-favicon-ico

@sbordet sbordet requested a review from joakime May 6, 2024 20:05
@sbordet
Copy link
Contributor

sbordet commented May 6, 2024

@joakime if you think x-icon is better, then let's close this, unless there is clear evidence otherwise.

@joakime
Copy link
Contributor

joakime commented May 6, 2024

The file extension '*.ico' can be either Microsoft icon, or PNG/JPG.

It's one of those things that will be application specific.

I think we should stick with whatever our default icon format is, and make sure it can be configured for the other mimetype if the application wants to use it.

@gregw
Copy link
Contributor

gregw commented May 7, 2024

@maffe Do you have a read world problem with the existing mapping? If not, we are inclined to leave it as is.

@joakime
Copy link
Contributor

joakime commented May 7, 2024

I think we should merge this.

Our own favicon.ico is reported as MS Windows icon resource

[jetty-12.0.x]$ find . -name "*.ico" -exec file {} \;
./jetty-ee10/jetty-ee10-demos/jetty-ee10-demo-jetty-webapp/src/main/webapp/favicon.ico: MS Windows icon resource - 1 icon, 16x16, 32 bits/pixel
./jetty-ee10/jetty-ee10-maven-plugin/src/it/jetty-start-gwt-it/beer-server/src/main/webapp/favicon.ico: MS Windows icon resource - 1 icon, 16x15, 32 bits/pixel
./jetty-ee8/jetty-ee8-maven-plugin/src/it/jetty-start-gwt-it/beer-server/src/main/webapp/favicon.ico: MS Windows icon resource - 1 icon, 16x15, 32 bits/pixel
./jetty-home/src/main/resources/modules/demo.d/root/favicon.ico: MS Windows icon resource - 1 icon, 16x16, 32 bits/pixel
./jetty-core/jetty-server/src/main/resources/org/eclipse/jetty/server/favicon.ico: MS Windows icon resource - 1 icon, 16x16, 32 bits/pixel
./jetty-ee11/jetty-ee11-runner/target/classes/org/eclipse/jetty/server/favicon.ico: MS Windows icon resource - 1 icon, 16x16, 32 bits/pixel
./jetty-ee11/jetty-ee11-demos/jetty-ee11-demo-jetty-webapp/target/jetty-ee11-demo-jetty-webapp-12.1.0-SNAPSHOT/favicon.ico: MS Windows icon resource - 1 icon, 16x16, 32 bits/pixel
./jetty-ee9/jetty-ee9-demos/jetty-ee9-demo-jetty-webapp/src/main/webapp/favicon.ico: MS Windows icon resource - 1 icon, 16x16, 32 bits/pixel
./jetty-ee9/jetty-ee9-nested/src/main/resources/org/eclipse/nested/favicon.ico: MS Windows icon resource - 1 icon, 16x16, 32 bits/pixel
./jetty-ee9/jetty-ee9-maven-plugin/src/it/jetty-start-gwt-it/beer-server/src/main/webapp/favicon.ico: MS Windows icon resource - 1 icon, 16x15, 32 bits/pixel

Also, if we ask Java's java.nio.file.Files.probeContentType(Path) we get ...

[jetty-12.0.x]$ jshell
|  Welcome to JShell -- Version 17.0.10
|  For an introduction type: /help intro

jshell> var ico = Path.of("./jetty-home/src/main/resources/modules/demo.d/root/favicon.ico")
ico ==> ./jetty-home/src/main/resources/modules/demo.d/root/favicon.ico

jshell> Files.probeContentType(ico)
$2 ==> "image/vnd.microsoft.icon"

jshell> 

Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

Unfortunately we cannot merge this PR as it currently stands, due to lack of signed ECA.

See: https://github.com/jetty/jetty.project/blob/jetty-12.0.x/CONTRIBUTING.md#eclipse-contributor-agreement

@maffe
Copy link
Author

maffe commented May 13, 2024

I will not create an Eclipse account (I don’t think I can receive e-mails at users.noreply.github.com and I don’t want to provide the information the Eclipse registration form requires), but feel free to do whatever you want with this trivial PR. I don’t think the tiny bit of knowledge it contains can be copyrighted.

@gregw
Copy link
Contributor

gregw commented May 14, 2024

I will not create an Eclipse account (I don’t think I can receive e-mails at users.noreply.github.com and I don’t want to provide the information the Eclipse registration form requires), but feel free to do whatever you want with this trivial PR. I don’t think the tiny bit of knowledge it contains can be copyrighted.

The knowledge might not be copyrighted, but this expression of it can be.

@joakime let's close this PR and ask a team member that had not participated in it to make the change.

@maffe thanks for the contribution of there idea if not the coffee itself.

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.

None yet

4 participants