The icon file attached to bug 160840 does not display correctly - there is a 1px skew per line (it is displayed correctly in IE6 and MS Windows Explorer if you want a proper look at it). *** Note that viewing it will crash the browser until the fix for patch 151154 is checked in or added to your tree ***

Seeking review. This patch works on the .ico file previously mentioned, and seems to be OK for other sites (but I've only browsed a few). Is there a set of test .ico files to try? It seems like there isn't a test set in the source tree.

Requesting review (cbiesinger@web.de, I believe you can r= ?)

hm, does SetAlphaData require the same fix?

I don't know if it requires it in this particular case, but it could be needed generally as the bug was in assuming the decoder and frame used the same buffer size. I think the safest assumption is that both provide a buffer of at least the minimum required size. I've knocked up a second go at the patch, but for some reason it doesn't seem to be linking (I'm getting no compile warnings but the debugger is stepping the old code on the original lines). I'll attach what I have as something has come up and I'm going to be away for a few days. Sorry, this happened after I asked for review - I would have waited if I'd known. At least you can see the approach I'm taking.

Original skew correction + factors out calculation of alpha buffer row size to allow different alpha buffer sizes in decoder and frame.

Are there any test cases for this? I don't know where to get/make .ico files with funky alpha data. Any idea which programs under W2K or Linux I could use?

*** Bug 166179 has been marked as a duplicate of this bug. ***

Comment on attachment 97445 [details] [diff] [review] Patch v2 ok... I had another look at this now... You should probably use bpr as the second argument in SetImageData. (and SetAlphaData. that's what the BMP Decoder does as well). Now, I don't think this is any different for the Alpha Data. So don't need the rowCopyLen variable there and can use bpr as well. And maybe you should not use an offset and add it to the pointer every time, but instead get a pointer and increase that in every iteration.

I think SetImageData should not use bpr since that refers to the row size in the frame buffer - using decodedLineLen means you are always copying exactly one row of data, nothing more and nothing less. If bpr != decodedLineLen then you will copy image data from following rows. I will put back the alpha data part if you like, but it seems that the decoder is then making the assumption that the frame implementation allocates data as the decoder does. Since they don't share that code, even if that assumption is true now (which I think it is, I have not checked) it may not be in the future. The bit I wrote would survive such a change. I'll agree about the pointer though :) I still have no .ico files with alpha data to test this patch though.

hmmm. you are right that data from other rows will be copied. however, that data would be ignored anyway. thinking about it, using bpr would cause an ABR at the last line (if bpr>decodedLineLen). so your patch is probably fine, if you address the pointer issue.

I am very confused. Patch v3 seems to build and new objects/dlls get created, but when I step the code I am seeing the symbols from v2? I have tried make from the top level and from lower directories, but nothing seems to help.

Comment on attachment 98467 [details] [diff] [review] Patch v3 +inline PRUint32 nsICODecoder::CalcAlphaRowSize() that won't work, actually. you need to put that into the .h file. I would actually put that into the class definition. I'm really sorry for not noticing that earlier... if you do that, you get r=biesi, provided you get sr=tor

> that won't work, actually. you need to put that into the .h file. I would > actually put that into the class definition. At the bottom of patch v3, the .h file has: + PRUint32 CalcAlphaRowSize(); Do you mean that it should be declared inline in the .h file? The other inine functions such as SetAlphaData are declared as inline in the .cpp file and without inline in the .h file.

> Do you mean that it should be declared inline in the .h file? yes. >The other >inine functions such as SetAlphaData are declared as inline in the .cpp file >and without inline in the .h file. really? that should probably be fixed as well, though not for this bug (unless you want to do that here)

Thanks for the reviews. I've determined that something in my build environment is definitely odd, as the img*.dll files from the component subdirectory are not being rebuilt (they were before patch so I see the symbols from v2). The compile does fail if I add nonsense to the source file, so it's valid source at least. Therefore, I have not been able to even see the effect of later patches. Could somebody compile this in and check it for me please, as I can't do so myself :(

patch compiles & works for me.

ok sorry but that was not what I meant :) I meant that the functions themselves should also be moved into the .h file rather than the .cpp.

Guess who's got his "C" head on still. I did tell myself I wouldn't commit sillies when contributing. So much for that :) This still compiles, still not producing dlls for me.

Comment on attachment 98618 [details] [diff] [review] Try again we're getting there :) you still need the "inline" in the class definition and there's no need to remove this comment: -// ---------------------------------------- -// Actual Data Processing -// ---------------------------------------- but, you have r=biesi now and need not attach a new patch for these changes. now you just need to get tor to sr :)

That comment seemed to apply to the helper functions more, and it looked odd sitting on top of the constructor. Why does the declaration in the class need to have "inline"? My Stroustrup (3rd ed) has something similar in section 10.2.9 (In-Class Function Definitions) which I referenced in a vain attempt to get version 5 right. That has a prototype "day" declared without it, and the definition following the class with it. There appears to be nothing in the Mozilla portability guide about it. I'm not asking to be smart, just so that I get it right on the first go next time.

>Why does the declaration in the class need to have "inline"? ok... I talked to scc now on irc who knows this stuff better than I :) and he said that you don't need the inline in the class declaration (if you do use the inline when implementing the function). however... I would still prefer it if the inline was there in the class declaration. it makes it clearer that the functions are inline.

OK, that makes sense. Do you still want the comment put back? And thanks for your patience :)

yes I still want it back :) but as I said, no need to attach a new patch.

hm actually some of the function could and should be shared between the bmp and ico decoder. I'll do that in a separate, yet to be filed, bug.

Comment on attachment 98618 [details] [diff] [review] Try again Leave the SetPixel* functions in the .h, but move the others back to the .cpp file (they're not called enough to make a difference).

(when moving them back, remove the "inline " part)

Comment on attachment 98933 [details] [diff] [review] Patch v6 looks great r=biesi

Comment on attachment 98933 [details] [diff] [review] Patch v6 sr=tor

ok, now you should just ask pavlov@netscape.com if the patch is ok with him (he's module owner and asked to be shown patches which will be checked in) (don't ask him via bugmail)

Just noting that I sent pavlov an email a few hours after the last comment was posted.

Comment on attachment 98933 [details] [diff] [review] Patch v6 r=pavlov

arunan: (btw, in case you'll create more bmp/ico/mng patches, stuart just said that mailing him for them is not necessary) do you have CVS access? if not, I can check this in in the next days.

I have no CVS access so please go ahead and check-in, increasing my total of accepted moz patches to: 2

And to cap off all my retardedness in this bug, it appears I should not have opened it as it seems to be a DUPE of bug 116778 :) I mean well.

*** Bug 116778 has been marked as a duplicate of this bug. ***

ah well, such things happen :) I'll check this in tomorrow, as the tree is closed right now.

checked in.