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

Support .ico #7666

Closed
paulrouget opened this issue Sep 18, 2015 · 15 comments
Closed

Support .ico #7666

paulrouget opened this issue Sep 18, 2015 · 15 comments
Assignees
@mbrubeck
Labels
A-content/images Interacting with images from web content I-enhancement No impact; the issue is a missing or proposed feature.

Comments

@paulrouget
Copy link
Contributor

Servo can't display hackernews' favicon:

  <img src="https://news.ycombinator.com/favicon.ico">
@pcwalton pcwalton changed the title Servo can't decode this image Support .ico Sep 18, 2015
@pcwalton
Copy link
Contributor

The problem is that we don't support .ico.

@paulrouget paulrouget mentioned this issue Sep 18, 2015
Closed
92 tasks
@jdm jdm added the A-content/images Interacting with images from web content label Sep 18, 2015
@paulrouget
Copy link
Contributor Author

Browser.html needs this to show favicons.

What should be done here? Add support to stb-image or use another library?

@pcwalton
Copy link
Contributor

Let's use another library. We're past due time to move off stb_image.

@waddlesplash
Copy link

See also: #3368.

@mbrubeck
Copy link
Contributor

mbrubeck commented Oct 5, 2015

PistonDevelopers/image does not support ICO either, so #3368 won't help directly. (But it will if we also work on adding ICO decoding to the image library.)

For browser.html in particular, @jdm mentioned we could use a decoder written in JavaScript that renders to canvas.

@mbrubeck mbrubeck self-assigned this Oct 6, 2015
@mbrubeck
Copy link
Contributor

mbrubeck commented Oct 7, 2015

Submitted a PR to add ICO support to the image crate: image-rs/image#450

@mbrubeck mbrubeck added the I-enhancement No impact; the issue is a missing or proposed feature. label Oct 8, 2015
@mbrubeck
Copy link
Contributor

mbrubeck commented Oct 8, 2015

With PR #7933 plus image-rs/image#450, Servo supports ICO images!

@bjwbell
Copy link
Contributor

bjwbell commented Oct 9, 2015

That's fantastic :-) You really got into it Matt.

On Thu, Oct 8, 2015 at 4:30 PM, Matt Brubeck notifications@github.com
wrote:

With PR #7933 #7933 plus
image-rs/image#450
image-rs/image#450, Servo supports ICO
images!

—
Reply to this email directly or view it on GitHub
#7666 (comment).

-- Regards
Bryan

@paulrouget
Copy link
Contributor Author

Thank you Matt :)

@waddlesplash
Copy link

#7933 has been merged, and so has image-rs/image#450, so as soon as Image releases v0.3.15 and/or Servo updates to the latest Git build, this'll be solved.

@paulrouget
Copy link
Contributor Author

#7933 has been merged, and so has image-rs/image#450, so as soon as Image releases v0.3.15 and/or Servo updates to the latest Git build, this'll be solved.

Is that happening soon?

@jdm
Copy link
Member

jdm commented Oct 19, 2015

Should be a straightforward ./mach cargo-update -p image.

@paulrouget
Copy link
Contributor Author

Updating to the latest version of image crashes Servo. Investigating.

@paulrouget
Copy link
Contributor Author

crash: image-rs/image#463

@paulrouget
Copy link
Contributor Author

Crash fixed. I guess we can now update. PR: #8119

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mbrubeck mbrubeck

Labels
A-content/images Interacting with images from web content I-enhancement No impact; the issue is a missing or proposed feature.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@mbrubeck @jdm @bjwbell @pcwalton @paulrouget @waddlesplash