222694 - moz-icon:s don't work in less than 16-bit colour, e.g. W2K Terminal Services

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[[e-mail]]

When moz-icon:s were created we didn't have an ico decoder so Scott wrote a
converter and decoder. Now that we have an ico decoder we don't need to convert
Windows icons any more.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

hm, you should probably disable building of nsIconDecoder.cpp too

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

-> patch author

Comment 4

[Ere Maijala (slow)]

Comment on attachment 133532 [details] [diff] [review]
Proposed patch

> result = GetIconInfo(sfi.hIcon, &iconInfo);
> if (result > 0)

I'd like to see correct types used. GetIconInfo() returns BOOL. 

> delete buffer;

You need |delete [] buffer;| to avoid memory leak.

Comment 5

[[e-mail]]

Attachment: Addressed review comments

Comment 6

[Ere Maijala (slow)]

Comment on attachment 133674 [details] [diff] [review]
Addressed review comments

r=ere

Comment 7

[[e-mail]]

Comment on attachment 133674 [details] [diff] [review]
Addressed review comments

sr hokey-cokey

Comment 8

[Scott MacGregor]

Neil,

It has probably been too long since I wrote this, but I wrote this initially so
we could display OS icons to next mail attachments and in the helper app dialog
based on the content type of the url. Each OS had a different native image
format for these images which I then decoded into RGB values for image lib to
display.

So this patch effects the windows icon decoder only. How do we render the image
now if you took out all of the code that converted it to something we internally
understand?

are you saying the .ico decoder for windows is now doing this for us? 

"just trying to understand what is going on"

Comment 9

[tor]

Comment on attachment 133674 [details] [diff] [review]
Addressed review comments

Back in mscott's sr court.

Comment 10

[[e-mail]]

Scott: yes; wasn't my initial description clear enough?

Comment 11

[[e-mail]]

biesi thinks this might also decode icons with alpha transparency but I've got
no way to test that...

Comment 12

[Scott MacGregor]

Comment on attachment 133674 [details] [diff] [review]
Addressed review comments

I'm assuming you have tested that the icons still render correctly in both the
helper app dialog (where we use a large icon) and in the mail message pane and
compose attachment bucket.

Comment 13

[[e-mail]]

Well, they do in 256 colours ;-)

Comment 14

[[e-mail]]

Fix checked in.