291381 - nsIconChannel fixes & enhancements for OS/2

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Rich Walsh]

This patch fixes color corruption seen in 256-color icons, adds a simple
full-size to mini icon converter, and yields more appropriate icons than the
current code.  It also greatly simplifies the icon-conversion code, eliminates
unnecessary functions, and reduces overhead.

Color corruption is caused by the use of an nsCString as an output buffer.  On
my system, it changes 0xF0 to 0xDA and 0xF1 thru 0xFF to 0x00.  It has been
replaced by a conventional byte buffer.  Inappropriate icons are often displayed
because the current code relies on a file's MIME-type.  Since most downloads are
described as "application/octet-stream", this produces the icon for an EXE.  The
revised code ignores the MIME-type and relies solely on the file's extension.

FYI...  I started working on nsIconChannel as part of a project to provide
thorough-going WPS integration.  A future enhancement will enable this code to
display whatever icon the WPS would display for a given file.  Since that
enhancement is still a work-in-progress while these revisions are ready to be
used, I'm submitting them now.  Further changes to implement that enhancement
will be trivial.

Comment 1

[Rich Walsh]

Attachment: fixes color corruption & simplifies code

Comment 2

[Rich Walsh]

Attachment: revised code

The diff is particularly unreadable - even Bugzilla makes a mess of it.  Here
are all of the substantive changes in one easy-to-read package.

Comment 3

[Mike Kaply [:mkaply]]

So am I to understand that you aren't using the 16x16 and 20x20 icons that are
in the icons anymore if needed? You shrink it yourself?

(this is based on your comments, I haven't read the code yet)

Comment 4

[Mike Kaply [:mkaply]]

Never mind, I just read and you use a mini if available.

We had a strange problem in the past that maybe you could help solve.

It's best that we always use the 16x16 icons (never 20x20) because the Mozilla
UI is designed around the fact that mini icons are 16x16. This affects things
like tree views where system icons are used and things like that.

Any idea how we can always get the 16x16 icon regardless of the system resolution?

Comment 5

[Peter Weilbacher]

Much of the code I don't understand (I still don't have a clue about graphics
programming), but you could use NS_OS_TEMP_DIR instead of the getenv bit in
GetFileIcon. An example is in widget/src/os2/nsSound.cpp:
   nsCOMPtr<nsIFile> soundTmp;
   rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR, getter_AddRefs(soundTmp));

Comment 6

[Mike Kaply [:mkaply]]

The temp dir stuff was a relic from my code, so I made that mistake originally -
probably a good idea.

Comment 7

[Rich Walsh]

Attachment: revised patch using NS_OS_TEMP_DIR

Comment 8

[Rich Walsh]

(In reply to comment #5)
> you could use NS_OS_TEMP_DIR instead of the getenv bit in
> GetFileIcon. An example is in widget/src/os2/nsSound.cpp:

Done.  I didn't like it either but I didn't know how to do it properly.

P.S. I always wondered how that mozsound.wav kept appearing in my temp dir - now
I know why, thanks to your reference.

Comment 9

[Rich Walsh]

(In reply to comment #4)
> you use a mini if available.

The shrink code is required when I fetch icons from the WPS because I cannot get
WinCreatePointerIndirect() to create a mini-icon - it completely ignores the
mini bitmaps.  When I put them in the full-sized fields, GPI stretches the image
by doubling everything.  The shrink code simply reverses the process and
produces an exact duplicate of the original.

> Any idea how we can always get the 16x16 icon regardless of the system
> resolution?

No.  Except for really mechanical stuff like the above, I'm out of my depth when
it comes to graphics (I'm a text guy, ya know?).  In any case, I doubt there's
an application level call available.  The very fact that SNAP has an environment
variable that lets you choose between small & large icons makes me think that
the decision as to which one to use is buried deep within PM's GRE.

Comment 10

[Mike Kaply [:mkaply]]

Comment on attachment 181558 [details] [diff] [review]
revised patch using NS_OS_TEMP_DIR

r=mkaply, sr=mkaply (platform specific)

Comment 11

[Mike Kaply [:mkaply]]

Fix checked in to trunk only.

Comment 12

[Mike Kaply [:mkaply]]

Rich,

I'm going to back this out. On at least two machines, it is causing crashes when
doing downloads.

The download I am testing with is:

http://www.innotek.de/products/ft2lib/ft2libgeneral_e.html

First link.

the NS_NewPipe is failing which probably implies some memory corruption going on....

Comment 13

[Rich Walsh]

(In reply to comment #12)
> The download I am testing with is:
> http://www.innotek.de/products/ft2lib/ft2libgeneral_e.html
> First link.

Not surprisingly, I can't duplicate it - I assume you mean the link to the v2.6
beta exe.  Does opening the d/l dialog cause it to crash?  Any idea of what
flavor(s) of Warp this occurs on?  (E.g. Does it happen on eCS with its
256-color icons or standard IBM versions with 16 colors - the code is different
for each.)

If this is something you've experienced, could you try to duplicate it using my
WPS-enabled version of Firefox?  (See my newsgroup posting for more details.)

    http://e-vertise.com/warpzilla/fx-wps-v1.zip

Comment 14

[Mike Kaply [:mkaply]]

I've recreated on two machines, both straight up OS/2 with SNAP graphics - one
is my machine at work, one is my machine at home. Both are Convenience Pack 2.

The crash is definitely as the download dialog is coming up as it goes to grab
icon info.

Comment 15

[Mike Kaply [:mkaply]]

I narrowed down the problem at least to a function - ConvertColorBitMap

http://lxr.mozilla.org/seamonkey/source/modules/libpr0n/decoders/icon/os2/nsIconChannel.cpp#476

If this function isn't called, we don't crash....

still looking.

Comment 16

[Mike Kaply [:mkaply]]

um, no it's convertmaskbitmap causing the problem

http://lxr.mozilla.org/seamonkey/source/modules/libpr0n/decoders/icon/os2/nsIconChannel.cpp#566

Comment 17

[Mike Kaply [:mkaply]]

In the bad call

pBMInfo->cx = 20
pBMInfo->cy = 40
pBMInfo->cBitCount = 1
bprIn = 4
lprIn = 1

outBuf is 1282 bytes per original allocation

inbuf is 240 bytes per original allocation

Comment 18

[Rich Walsh]

Mike,

Thanks for your efforts.  My testing with XGA icons wasn't as thorough as it
should have been.  Perhaps my assumptions about padding are incorrect.  I'll
check this out later today.

Comment 19

[Rich Walsh]

> Perhaps my assumptions about padding are incorrect.

Yep.  20x20 16-color icons have two bytes of padding that ConvertColorBitMap()
fails to ignore.  This causes a buffer overrun of 240 bytes.  I'll fix this and
submit a patch on Wednesday.  Do you want a diff for just this fix or a revised
version of the original patch?

Comment 20

[Rich Walsh]

Correction - that was two bytes of padding _per row_ that it fails to ignore.

Comment 21

[Peter Weilbacher]

As the first version is still in CVS Mike most likely needs just an additional
patch to correct the problem.

Comment 22

[Mike Kaply [:mkaply]]

Given that you have an idea of the fix, I won't back this out - please just
submit a patch on what is in CVS.

Thanks!

Comment 23

[Rich Walsh]

Attachment: fix for 16-color 20x20 icons

Comment 24

[Rich Walsh]

I tested this latest patch with 16 & 256 color icons in both VGA & XGA sizes.  I
also tested with icons that had no mini-icon counterpart to ensure the shrink
code worked as well at all sizes & color-depths.

Comment 25

[Mike Kaply [:mkaply]]

checked in.

Thanks!