182621 - (wbmp) WAP BMP (wbmp) format support

Status: RESOLVED WONTFIX

(Core :: Graphics: ImageLib, enhancement)

Description

[Manko]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2) Gecko/20021126
Build Identifier: 

According bug 35995 which near to be fixed (I hope), we need for support WAP BMP
images. See section 6 of PDF document at URL for wbmp specification

Reproducible: Always

Steps to Reproduce:

Comment 1

[Manko]

Attachment: Sample WBMP image

Comment 2

[Michael Lefevre]

yet another image type... if mozilla is going to do WML, then I guess this is
needed. confirming.

Comment 3

[Arunan Balasubramaniam]

Attachment: Make mozilla aware of image/vnd.wap.wbmp.

Comment 4

[Arunan Balasubramaniam]

Attachment: WBMP decoder

Comment 5

[Arunan Balasubramaniam]

Attachment: WBMP decoder

Comment 6

[Arunan Balasubramaniam]

I had a go at this not actually expecting to get anywhere, but here is something
that displays .wbmp files.  w00t!  It's the BMP decoder hacked and poked until
it produced an image from the test file.  This is just dumping my work in
progress, as I don't know if I'm going to have much free time for the next week
and a bit.

Issues:

1) The two enums defined have a very similar value - DECODING_FAILED and
DECODE_FAILED, just spotted now after uploading here.
2) This is in the BMP decoder's subdirectory as I just copied those files and
was hoping to re-use some common code.
3) Nothing added to imgLoader.cpp; don't know if I could/should or not.
4) Forgot how to create a new CID so I just incremented the BMP one.
5) I didn't know what to do about the copyright notice as it's mostly not my
code.  I think the thing to do is leave the copyright as it is in the BMP
decoder (cbiesi) and add myself as a contributor.

Comment 7

[Arunan Balasubramaniam]

Comments welcome :)

Comment 8

[Arunan Balasubramaniam]

Attachment: modules\libpr0n\decoders\bmp\Makefile

Updated makefile for BMP decoder directory.

Comment 9

[Arunan Balasubramaniam]

Attachment: nsWBMPDecoder.cpp

Comment 10

[Arunan Balasubramaniam]

Attachment: nsWBMPDecoder.h

Comment 11

[Arunan Balasubramaniam]

Just updated the files:

  - rename enums to avoid clash
  - change to 2 space identation as style guide says
  - found guidgen to create CID
  - filled in copyright notice

So if it is OK to be in the BMP directory, I think this can be reviewed.

Comment 12

[Manko]

Arunan, my regards :)

Please, request a review via Edit link at your attachment. See
http://bugzilla.mozilla.org/flag-help.html for more information.

Comment 13

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

>modules\libpr0n\decoders\bmp\Makefile

um. no. firstly, modify Makefile.in, secondly, please make this a patch.

Comment 14

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

ok now to the patch

it does not belong in the bmp directory. it has nothing to do with the BMP
Decoders, except the name. please make a new directory.

and yeah... the main issue will be that you need to get the imagelib module
owner to approve this. that is [e-mail] (he doesn't read bugmail)

Now. thing is, he is opposed to adding new image decoders to imagelib, last I
heard... but feel free to mail him and ask.

the actual code looks ok on first sight.
if you want a real review, just ask.

Comment 15

[Arunan Balasubramaniam]

Thanks for the comments.  I guess Makefile was left in my tree from the nmake ->
gmake switch.  Should have twigged it wasn't in CVS when diff -u would not work,
which was why I attached the whole thing.

I will move the files to a WBMP directory and redo the patches.  Then I'll ask
for review before approaching pavlov.

Comment 16

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

severity -> enhancement, as it should be

Comment 17

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

ah yeah more comments
o) yes you should change imgLoader.cpp, to look for the signature of WBMP files
o) create a CID by connecting to irc.mozilla.org and /msg botbot uuid?
please do not randomly invent CIDs.
o) yes, if you are modifying a file, it is correct to only add yourself as a 
contributor rather than changing the copyright

Comment 18

[Arunan Balasubramaniam]

The CID was created using guidgen from MSVC, which I thought was OK from
http://www.mozilla.org/docs/modunote.htm (section "About nsIIDs and nsCIDs").

Would it be better to re-attach the source and make files for a new "wbmp"
directory as a zip file or just to attach a new makefile.in?

From looking at the code, I think that the appropriate check to add to
imgLoader.cpp would be a check for the file starting with 0x00 and then check
the second byte by ANDing with 0x9F, expecting 0x00 result.  This check would
follow the .ICO check at line 752 as ICO checks for 0x00 0x00 which could match
but an ICO is more likely, and the following checks don't match for 0x00 in the
first byte.

Comment 19

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

>The CID was created using guidgen from MSVC,

sure, that's fine. I was referring to your earlier "incremented bmp decoder's
cid by one" statement.

makefile.in patch is enough

yeah, that approach for the imgloader change sounds right

Comment 20

[Arunan Balasubramaniam]

Attachment: modules\libpr0n\decoders\wbmp\Makefile.in

Just twigged that Makefile is generated :)
The severity change was an accident - apologies.

Comment 21

[Arunan Balasubramaniam]

Attachment: Make mozilla aware of image/vnd.wap.wbmp

Comment 22

[Arunan Balasubramaniam]

Gah, sorry for spam.  Had my head in bug 97777 where I was looking for sr=

Comment 23

[Manko]

Nice work, Arunan :))

Can you any idea about bug 35995? If yes, welcome to discussion there.

Comment 24

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

Comment on attachment 108213 [details] [diff] [review]
Make mozilla aware of image/vnd.wap.wbmp

you need to change allmakefiles.sh as well

"wbmp" this is the correct file extension for WBMP files? Ie. it is not a
3-letter one?

Comment 25

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

/home/chb/mozilla/modules/libpr0n/decoders/wbmp/nsWBMPDecoder.cpp: In method
`nsresult nsWBMPDecoder::ProcessData(const char *, unsigned int)':
/home/chb/mozilla/modules/libpr0n/decoders/wbmp/nsWBMPDecoder.cpp:240: warning:
comparison between signed and unsigned
/home/chb/mozilla/modules/libpr0n/decoders/wbmp/nsWBMPDecoder.cpp:286: warning:
comparison between signed and unsigned


please fix that

Comment 26

[Arunan Balasubramaniam]

Attachment: Patch to mozilla\allmakefiles.sh

Comment 27

[Arunan Balasubramaniam]

Google says:

1) Opera apparently supports WBMP - http://www.opera.com/docs/specs/

2) Most sites think the extension is .wbmp but one suggests it as .wbm as well
as .wbmp

The specification does not mention a standard file extension, only the MIME type.

BTW, all this has only been compiled and run on W2K.

Comment 28

[Manko]

Sorry for spam.

Arunan, it looks as if I cannot send email to you. I would like if you read bug
35995 comment 30 and tell your considerations. Thanks beforehand! :)

Comment 29

[Arunan Balasubramaniam]

Attachment: nsWBMPDecoder.cpp

Adds a cast to remove a warning.

Comment 30

[Arunan Balasubramaniam]

Attachment: nsWBMPDecoder.h

Change mCurLine to PRUint32 to remove warnings.

Comment 31

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

Comment on attachment 109435 [details]
nsWBMPDecoder.h

inline nsresult SetPixel

hm... you could make this void...
I think it is void in the BMP Decoder as well now.

inline WbmpIntDecodeStatus DecodeEncodedInt

what are you inlining this for - you have a pretty long loop in there, the wins
from inlining will be very small.

  char encodedByte;
why are you declaring this outside the loop?
Same for newFieldVal.

Actually, is there a reason for using newFieldVal rather than directly using
aField?

Comment 32

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

Comment on attachment 109434 [details]
nsWBMPDecoder.cpp

you should init. mDecodingState in ::Init I think... not sure if objects are
supposed to be reusable...

  if (mDecodingState != DECODING_IMAGE_DATA) {

I don't think this optimization is worth the reduced understandability of the
code.

    // For now, ignore ext header field as it is not present in well defined
type 0 WBMP.

what happens if this field is present?

      WbmpIntDecodeStatus widthReadResult = DecodeEncodedInt (mWidth, aBuffer,
aCount);

What happens if this returns INT_PARSE_IN_PROGRESS - bad things, don't they?

    PRUint32 toCopy;
you're again declaring a variable outside the loop for no good reason.
(does the bmp decoder do the same things?)

Comment 33

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

Comment on attachment 108262 [details] [diff] [review]
Patch to mozilla\allmakefiles.sh

this attachment (allmakefiles) must have r=cls (but needs no super-review)

Comment 34

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

Comment on attachment 108212 [details]
modules\libpr0n\decoders\wbmp\Makefile.in

this one looks good.

Note that this review is _only_ valid if you get the module owner
([e-mail] last I checked) to approve adding WAP BMP support to
imagelib..

Comment 35

[Jim Dunn]

It is me now... see bug 185140
I will get back to you shortly...

Comment 36

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

Comment on attachment 108213 [details] [diff] [review]
Make mozilla aware of image/vnd.wap.wbmp

hm, yeah, this also looks good with the same limitation as above

Comment 37

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

Comment on attachment 108213 [details] [diff] [review]
Make mozilla aware of image/vnd.wap.wbmp

hm, yeah, this also looks good with the same limitation as above

Comment 38

[Arunan Balasubramaniam]

Comment on attachment 109435 [details]
nsWBMPDecoder.h

> inline nsresult SetPixel
> 
> hm... you could make this void...
> I think it is void in the BMP Decoder as well now.

  Yes, it has changed in the BMP decoder (since I copied it :) and will here too.

> inline WbmpIntDecodeStatus DecodeEncodedInt
> 
> what are you inlining this for - you have a pretty long loop in there, the
> wins from inlining will be very small.

  inline removed.

>   char encodedByte;
> why are you declaring this outside the loop?
> Same for newFieldVal.

  Just personal style, changed now.

> Actually, is there a reason for using newFieldVal rather than directly using
> aField?

  Having the new and previous value lets you check for overflow without knowing
the size of the data type "if (newFieldVal < aField)".

> (From update of attachment 109434 [details])
> you should init. mDecodingState in ::Init I think... not sure if objects are
> supposed to be reusable...

  Moved.

>  if (mDecodingState != DECODING_IMAGE_DATA) {
>
> I don't think this optimization is worth the reduced understandability of the
> code.

  I will keep the if and make the code clearer by putting the if's contents
in a parse_wbmp_header function like in the BMP decoder.

>     // For now, ignore ext header field as it is not present in well defined
> type 0 WBMP.
>
> what happens if this field is present?

  If the ext header field is present, then bit 7 of the fix header field will be
set.  We check for this by ANDing the fix header field with 9F and checking that
Bits 7 and 4-0 are 0 as we expect.  If they are not (includes the case where
the header is present), we error out.
  This should be moot anyway as we also check for type 0 ealier, and at the
bottom of the specification it says a "well defined" type 0 will have all those
bits set to 0.
  I will add a comment to the code to say why the ext header should not be there.

>      WbmpIntDecodeStatus widthReadResult = DecodeEncodedInt (mWidth, aBuffer,
> aCount);
>
>What happens if this returns INT_PARSE_IN_PROGRESS - bad things, don't they?

  If INT_PARSE_IN_PROGRESS is returned, then the decoding state is not changed
and the flow of execution should drop to the bottom (as it will not match
further states) and return NS_OK.  So, I don't think bad things happen.
  Having just written that, I see I should have just made the return NS_OK
explicit in a final else clause, which is far clearer.  I'll do that.

>     PRUint32 toCopy;
> you're again declaring a variable outside the loop for no good reason.
> (does the bmp decoder do the same things?)

  That bit is as stolen from the BMP decoder (phew!).  Changed now.

Comment 39

[Arunan Balasubramaniam]

Sorry for spam, but to clarify:

> Having the new and previous value lets you check for overflow without knowing
> the size of the data type "if (newFieldVal < aField)".

  Although we know it's a PRUint32, by doing it this way we don't have to store
the number of bytes parsed if we only receive part of the encoded int field at
a time.

Comment 40

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

>  inline removed.

I hope that includes moving it to the .cpp file.

>  I will keep the if and make the code clearer by putting the if's contents
>in a parse_wbmp_header function like in the BMP decoder.

I didn't mean that
This if just contains several other ifs who test the same variable. so no need
to test the if just there.

>it says a "well defined" type 0 will have all those
>bits set to 0.

Well, you should never trust input data... not showing the image is OK for
invalid images; but if you would crash, that would not be ok - this is why I asked.

>  If INT_PARSE_IN_PROGRESS is returned, then the decoding state is not changed
>and the flow of execution should drop to the bottom 

Yes, I knew that; sorry I should have been clearer. I meant - when the next part
of the integer will later arrive, will it work as expected? Ie. the variable
contains the full integer later?

Comment 41

[Arunan Balasubramaniam]

> I hope that includes moving it to the .cpp file.

  It helps it compile :)

> I didn't mean that
> This if just contains several other ifs who test the same variable. so no need
> to test the if just there.

  I know you didn't mean that, but it seems a better solution as the code is
overall clearer by factoring out the header parts (as the BMP decoder does).
  By doing that one test you eliminate lots of tests you know will fail in
the usual case, as the header is small compared to the image data.
  I have created ProcessHeaderData already but will put the code back into
ProcessData if you prefer.

> Well, you should never trust input data... not showing the image is OK for
> invalid images; but if you would crash, that would not be ok - this is why I
> asked.

  That's OK, we're not trusting the data unless it is exactly as specified.
So it's explicitly checking it's a good wbmp instead of assuming.

> Yes, I knew that; sorry I should have been clearer. I meant - when the next
> part of the integer will later arrive, will it work as expected? Ie. the
> variable contains the full integer later?

  It should work, as we store the running total.  I'll redo my test code for
that function to double check as I did modify it since I wrote it's test.

  Thanks for all the comments.  I'll create some more corrupted images and a
test page to display them so this code can be confirmed to not crash on bad
input data.  I don't know how to easily test image data being received in
fragments, but I will redo the harness for DecodeEncodedInt.

Comment 42

[Manko]

This bug is still an enhancement anyway.

Severity "trivial" means that bug is a slight cosmetic problem with an existing
Mozilla trunk code.

The rest of work at this bug is, I hope, indeed trivial (thanks!), but this
doesn't mean this bug to be trivial as a whole :)

Comment 43

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

> I have created ProcessHeaderData already but will put the code back into
> ProcessData if you prefer.

ok, thanks for the explanation, ProcessHeaderData seems fine to me.


so... will you attach the current version of the .cpp/.h?

Comment 44

[Arunan Balasubramaniam]

I will try to create the test image files before attaching new versions of the
source as I don't really want to ask for review and then test and find a bug
which means I have to add them yet again and have them reviewed again.

Comment 45

[Arunan Balasubramaniam]

Attachment: Zip of test files.

Comment 46

[Arunan Balasubramaniam]

Attachment: Zip of more test files

Comment 47

[Arunan Balasubramaniam]

Attachment: nsWBMPDecoder.cpp

Comment 48

[Arunan Balasubramaniam]

Attachment: nsWBMPDecoder.h

Comment 49

[Arunan Balasubramaniam]

Oh, it was me who changed the severity again ... <sighs> sorry again.  I'll edit
my bugzilla prefs to not filter my own changes so I'll notice if/when it happens
again.

I know how this Internet thing works, honest.

<Double checks it still says enhancement>

Comment 50

[Manko]

Attachment: Screenshot for multi-byte width/height WBMP

This is a screenshot from Opera 6.05 and IrfanView for your multi-byte files.
Both viewers show same image, thus it seems to be correct.

What output produces your decoder?

Comment 51

[Arunan Balasubramaniam]

The images are the same in the local decoder.  The multi-byte tests are just to
ensure that multi-byte ints are decoded correctly.  The width one looks the way
it does as it is just the same image data four times so the first "wide" row is
the first four "normal" rows in a line, etc.  The other tests prove that the
image data would be decoded correctly, as do your screenshots (thanks).

<checks severity field>

Comment 52

[Manko]

Nice work :)

Waiting for review.

Comment 53

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

Comment on attachment 109786 [details]
nsWBMPDecoder.cpp

#include "nsIImage.h"
#include "ImageLogging.h"
#include "imgIContainerObserver.h"

please verify that you really need these includes, and remove them if not. I
also now notice that the "string" line in Makefile.in can and should be
removed.

      if ((rowSize - mRowBytes) == 0) {
you could write this as
if (rowSize == mRowBytes)

	    SetPixel(d, (PRBool)(idx > 0));
uh is the PRBool cast really necessary? and if it is, the >0 part is not
necessary. SetPixel does not explicitly compare to PR_TRUE/FALSE, only does if
(aPixelWhite).

The rest looks good, but could you please add a comment to DecodeEncodedInt
that describes what exactly the function does, and how these encoded integers
look like?

Also, this function doesn't look like it needs to be a member function, just
make it a static function in the .cpp file.

with these changes you can get r=biesi, but please attach a new patch

Comment 54

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

Comment on attachment 109788 [details]
nsWBMPDecoder.h

  DECODING_EXT_HEADER,
you never use this enum value... I would remove it...

#define WBMP_GFXFORMAT gfxIFormats::BGR
is this used in more than one place?
if not, there's no need to use this define and you could just #ifdef the code
in the .cpp file.

Ah... I see you did comment what DecodeEncodedInt does... but you do not make
it clear that if parse is in progress, another call of this function with the
same aField parameter does "the right thing".


again, r=biesi with these changes if you attach a new patch.

Comment 55

[Arunan Balasubramaniam]

Attachment: modules\libpr0n\decoders\wbmp\Makefile.in

Removed string from REQUIRES.

Comment 56

[Arunan Balasubramaniam]

Looking again at my test cases, I am wondering if it is OK that some of the
image cells are truly empty and some have no visible content (as if &nbsp). 
Surely Mozilla should produce the same result for any type of invalid image? 
What would the correct result be for an incorrect image?

Comment 57

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

Comment on attachment 110615 [details]
modules\libpr0n\decoders\wbmp\Makefile.in

better, thanks

Comment 58

[Arunan Balasubramaniam]

Attachment: modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp

Comment 59

[Arunan Balasubramaniam]

Attachment: modules\libpr0n\decoders\wbmp\nsWBMPDecoder.h

Comment 60

[Stuart Parmenter]

I don't really know why we need this.. but has anyone looked at how big the 
libraries are?  If it isn't very big, I probably won't be too against putting 
it in to the tree.

Comment 61

[Arunan Balasubramaniam]

Attachment: Size of DEBUG .obj files

This is the size of the debug object files in my working tree.	It looks like
the WBMP decoder will probably be just a little bigger than the XBM decoder,
however big that is.

Comment 62

[Manko]

Stuart, this issue appeared as consequence of bug 35995. I know many people to
want see WAP/WML pages in plain HTML browser and they would use for it open
source browser instead of Opera. They want see not only text but WBMP images ;)

Additional code for WBMP support is IMHO not too big.

Comment 63

[Arunan Balasubramaniam]

Aside from the requested changes, I also changed the call to SetPixel:

        while (lpos < mWidth) {
          PRInt8 bit;
          PRUint8 idx;

          for (bit = 7; bit >= 0; bit--) {
            if (lpos >= mWidth)
              break;
            idx = (PRUint8)((*p >> bit) & 1);
            SetPixel(d, (PRBool)(idx > 0));
            ++lpos;
          }
          ++p;
        }

  became

        while (lpos < mWidth) {
          PRInt8 bit;

          for (bit = 7; bit >= 0; bit--) {
            if (lpos >= mWidth)
              break;
            PRBool pixelWhite = (*p >> bit) & 1;
            SetPixel(d, pixelWhite);
            ++lpos;
          }
          ++p;
        }

  so it is only for this change I re-requested r=.

Comment 64

[Arunan Balasubramaniam]

Comment on attachment 108262 [details] [diff] [review]
Patch to mozilla\allmakefiles.sh

Requesting r= on the same "if module owner says OK" basis as the other patches.

Comment 65

[Jim Dunn]

sr=[e-mail] (NOTE: you still need r=

Comment 66

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

Comment on attachment 108213 [details] [diff] [review]
Make mozilla aware of image/vnd.wap.wbmp

fyi - this patch has just bitrotted due to the checkin for bug 169304

Comment 67

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

Comment on attachment 110663 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.h

  nsresult ProcessHeaderData(const char*& aBuffer, PRUint32& aCount);
please describe in the comment for this function what happens to aBuffer and
aCount.
E.g. "After returning, aBuffer will point after the bytes used by this
function, and aCount will be decremented by the number of used bytes."

and a comment above SetPixel would be nice too.

r=biesi with that change

Comment 68

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

um. jdunn: you are not listed at http://www.mozilla.org/hacking/reviewers.html
as a super-reviewer?

Comment 69

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

Comment on attachment 110662 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp

  NS_INIT_ISUPPORTS();
please remove this line, it's deprecated.

rest looks ok, r=biesi
no need for a new patch (just remove that line before checking in)

Comment 70

[Arunan Balasubramaniam]

> no need for a new patch (just remove that line before checking in)

Since I don't have check-in rights shall I attach a new version for whoever
checks this in for me?

Comment 71

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

if you want :) but I'd say that whoever checks this in can also remove that line.

Comment 72

[Arunan Balasubramaniam]

Attachment: modules\libpr0n\decoders\wbmp\Makefile.in

Bug 169304 changed each decoder's Makefile.in by removing the EXPORT_LIBRARY
line.

Comment 73

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

Comment on attachment 111283 [details]
modules\libpr0n\decoders\wbmp\Makefile.in

looks good

Comment 74

[Arunan Balasubramaniam]

Attachment: modules\libpr0n\decoders\wbmp\nsWBMPDecoder.h

Comment 75

[Arunan Balasubramaniam]

Attachment: Build changes

Changes for build (I believe to be reviewed by seawood - [e-mail]?)

Comment 76

[Arunan Balasubramaniam]

Attachment: Make mozilla aware of image/vnd.wap.wbmp

Code changes to make moz show wbmp love.

Comment 77

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

Comment on attachment 111724 [details] [diff] [review]
Make mozilla aware of image/vnd.wap.wbmp

this looks good
(wait, we have two nsContentDLF? someone ought to change that, really. nothing
you need to worry about, though)

yes, the build changes should be reviewed by cls - [e-mail] iirc

Comment 78

[hacker formerly known as [e-mail]]

Comment on attachment 111723 [details] [diff] [review]
Build changes

I'll r= the build changes after pavlov has given module owner approval.

Comment 79

[Stuart Parmenter]

Comment on attachment 111723 [details] [diff] [review]
Build changes

yeah sure

Comment 80

[hacker formerly known as [e-mail]]

Comment on attachment 111723 [details] [diff] [review]
Build changes

r=cls

Comment 81

[Arunan Balasubramaniam]

Um, do we have sr= from comment 65 ?  Bug 185140 would appear to make it so.  I
read in a blog that Pavlov is working on other things anyway :)

Comment 82

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

super-reviewers are listed at http://www.mozilla.org/hacking/reviewers.html
jdunn is not listed there

oh and btw, if you want to check this in before the 1.3 branch, you must get
approval from drivers.

Comment 83

[Arunan Balasubramaniam]

Attachment: modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp

Removed the NS_INIT_ISUPPORTS() line for the super reviewer.

Comment 84

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

Comment on attachment 112275 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp

the only change to the last patch is the NS_INIT_ISUPPORTS removal, right?
if so, r=biesi

Comment 85

[Arunan Balasubramaniam]

> the only change to the last patch is the NS_INIT_ISUPPORTS removal, right?

A newline crept in somehow at the end of ProcessData too, but the only code
change is the NS_INIT_ISUPPORTS removal.

This seems a low risk patch so I'll request drivers approval after getting sr=.

Comment 86

[Manko]

Critical means that this bug is causing Mozilla to be crashing, hanging,
security holing etc.

So, changing severity back ;)

Anyway, thank for your amazing work :))) I still hope for your contibution for
bug 35995.

Comment 87

[tor]

FYI: I'm not going to have a chance to sr this until next week.

Comment 88

[Arunan Balasubramaniam]

Sorry for inadvertently changing the severity again - I think I am a victim of
bug 33732 (mouse wheel scrolls list boxes even when outside them).  I have
disabled Moz in the wheel's list of applications to try to stop this happening.

tor: thanks for letting us know you will get to this later, rather than leaving
us wondering.

Comment 89

[Manko]

Hope to have this in 1.4 alpha.

Comment 90

[Asa Dotzler [:asa]]

blocking1.4a- because we wouldn't block the release for this change. 

Comment 91

[Arunan Balasubramaniam]

The patches attached to this have been broken since the trunk opened for 1.4
alpha anyway.  I have fixed them locally but didn't bother updating the bug in
case they broke again before they got super-reviewed.  I'll attach them if/when
requested.

I would think from the sentiments expressed in bug 197530 and bug 195280 that
this has little chance of landing on the trunk ever.  I think the chances would
be improved if image decoders were entirely pluggable (see
http://bugzilla.mozilla.org/show_bug.cgi?id=197530#c3) but not by much and from
a very small starting point in this case.

I am not moaning about this as I understand the need to keep minority features
out of the main build.  It would be nice if this could go in to the tree
disabled so people who wanted to play with WML could enable it for themselves in
their own builds.  And if it makes any difference, I would be happy to be
default owner for any and all bugs against this decoder.  My email is monitored
at least daily (except holidays :).

Comment 92

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

> if image decoders were entirely pluggable

see also bug 41333, esp bug 41333 comment 27,  and I think that for "normal" use
of wbmp images, the current level of pluggability suffices - all that (maybe)
wouldn't work is wbmp images for which no mimetype is available (e.g. FTP or
local files)

Comment 93

[tor]

brendan, chofmann: do you care about this for minimo?

Comment 94

[chris hofmann]

no specific requirement from any of the vendors we have talked to, but it sounds
like it might be good to get this in the tree disabled for the default build,
but we could get some folks to try it out and do some testing with a minimo build.

Comment 95

[Arunan Balasubramaniam]

I would be happy to get this working with current CVS if wanted.

Comment 96

[Arunan Balasubramaniam]

Attachment: modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp Fixes a bug in the multi-byte integer decoder

Comment 97

[Arunan Balasubramaniam]

Attachment: Build changes for WBMP

Comment 98

[Arunan Balasubramaniam]

Attachment: Make mozilla aware of image/vnd.wap.wbmp and WBMP decoder

Comment 99

[Arunan Balasubramaniam]

Attachment: XPFE changes

Seems to work, but I have just set and unset the WBMP checkbox.

I am not sure if the change to xpfe/components/build/nsModule.cpp should be in
this patch or patch 145694 "Make mozilla aware...".

Comment 100

[Arunan Balasubramaniam]

Don't know who to request reviews for the build changes and the XPFE changes
from, or any super reviews. Could some kind soul set those requests for me please?

Comment 101

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

Comment on attachment 145695 [details] [diff] [review]
XPFE changes

requesting review from neil... (not my patch)

+  deleteThisFile("Components", "wbmpdecoder2.shlb");

these lines are not necessary. such lines are here for upgrading from older
versions, because in current versions the image decoders are all part of
libimglib2.so

Comment 102

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

Comment on attachment 145691 [details]
modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp

Fixes a bug in the multi-byte integer decoder

/* ----- BEGIN LICENSE BLOCK -----

why are you using --- now instead of ***?

+  // ReadSegments does not return the error returned from ReadSegCb, so
+  // check for decoding errors here if we get a success return code.
+//  if (NS_SUCCEEDED(rv) && (mDecodingState == DECODING_FAILED))
+//    return NS_ERROR_FAILURE;

why is this commented out?
ah... looks like you're checking for this in ProcessData now. why not remove
this commented-out code, then?

+    if (aField & (0xFE << (8 * (sizeof (aField) - 1)))) {

hm... why not:
if (aField & 0xFE000000)
?
a PRUint32 will always be 32 bit large, hence the name...


Why are you no longer setting mDecodingState before returning an error? not
like it really matters, granted.

Comment 103

[[e-mail]]

Comment on attachment 145695 [details] [diff] [review]
XPFE changes

FE bits look ok but I don't have a PC to test on... passing the buck on the
build stuff too.

Comment 104

[Arunan Balasubramaniam]

Thanks for the quick replies.

> why are you using --- now instead of ***?

That came from diffing against the BMP decoder. I copied the block across to
make the diff clearer. Looking at
http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c , I realise that
was a mistake as I *had* used the official boilerplate before.

> +  deleteThisFile("Components", "wbmpdecoder2.shlb");

OK.

> +//  if (NS_SUCCEEDED(rv) && (mDecodingState == DECODING_FAILED))
> +//    return NS_ERROR_FAILURE;

Ack, this was an attempt to have mozilla show the broken image icon, copied from
either the GIF or JPEG decoder. It didn't work, so I should have removed it.

> hm... why not:
> if (aField & 0xFE000000)
> ?
> a PRUint32 will always be 32 bit large, hence the name...

My paranoia from watching previous code I worked on evolve. You can change the
size aField to any type and the code would still work. But 0xFE000000 is
certainly clearer :)

> Why are you no longer setting mDecodingState before returning an error? not
> like it really matters, granted.

Failed return codes get caught in ReadSegCb and mDecodingState is set to
DECODING_FAILED in one place instead of many.

So, would you like me to attach new versions of the patches for any/all of the
noted points?

1) Could put the license block back to the original
2) Remove deleteThisFile("...
3) Remove the commented out code in the decoder
4) Use a more obvious/less paranoid bit mask

Or does review+ mean you are happy to leave them or modify them yourself?

Comment 105

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

> So, would you like me to attach new versions of the patches for any/all of the
> noted points?

please do, for the patch that I marked review+ on... for the 2) part I'd wait
until bsmedberg comments on the patch.

> 1) Could put the license block back to the original
> 2) Remove deleteThisFile("...
> 3) Remove the commented out code in the decoder
> 4) Use a more obvious/less paranoid bit mask

Comment 106

[Arunan Balasubramaniam]

Attachment: modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp after review

License file back.

I put the commented out lines in WriteFrom() back in. Having that code recreate
the error return code stops an assertion at
netwerk/base/src/nsInputStreamPump.cpp line 451 from firing.

Bit mask now more obvious.

Comment 107

[Benjamin Smedberg]

Comment on attachment 145695 [details] [diff] [review]
XPFE changes

I'm pretty sure all of the "deleteThisFile" calls in the installer .jst files
are unnecessary. They are only necessary if there was previously a wbmp
component and now there isn't.

Comment 108

[Arunan Balasubramaniam]

Ah, so those lines are only to clean up installs from before the decoders got
combined in Bug 168048?

Shall I reattach the patch with the .jst changes chopped off?

Comment 109

[Aaron Kaluszka]

What's the status of this bug?  Patches were reviewed, but that was over a year
ago...

Comment 110

[Alfred Kayser]

This really probably needs to be updated after the Cairo changes

Comment 111

[Matthew Wilson]

Updated versions against current trunk are checked in at http://www.mozdev.org/source/browse/wmlbrowser/extensions/wbmp/

Comment 112

[d]

What's going on here? Is it going to be included or not?

Comment 113

[Joe Drew (not getting mail)]

The patches as-is aren't compatible with current Gecko, and there's not a pressing need to support wbmp any more. Feel free to reopen this if you really feel like I'm wrong here.

Comment 114

[John Drinkwater (:beta)]

Bug 847310 kinda changed this to DUPLICATE FIXED.