Bug 182621 (wbmp)

WAP BMP (wbmp) format support

RESOLVED WONTFIX

Status

()

enhancement
RESOLVED WONTFIX
17 years ago
3 years ago

People

(Reporter: sinchi, Assigned: foss)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.4a -

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(11 attachments, 20 obsolete attachments)

460 bytes, image/vnd.wap.wbmp
Details
10.96 KB, application/octet-stream
Details
2.67 KB, application/octet-stream
Details
55.01 KB, image/png
Details
2.33 KB, text/plain
Details
2.07 KB, text/plain
Biesinger
: review+
Details
4.84 KB, text/plain
Biesinger
: review+
Details
69.48 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
7.28 KB, patch
Biesinger
: review+
Details | Diff | Splinter Review
21.49 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
12.38 KB, text/plain
Biesinger
: review+
Details
Reporter

Description

17 years ago
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:
Reporter

Updated

17 years ago
Blocks: wml
Reporter

Comment 1

17 years ago
Posted image Sample WBMP image

Comment 2

17 years ago
yet another image type... if mozilla is going to do WML, then I guess this is
needed. confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee

Comment 4

17 years ago
Posted file WBMP decoder (obsolete) —
Assignee

Comment 5

17 years ago
Posted file WBMP decoder (obsolete) —
Assignee

Comment 6

17 years ago
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.
Assignee: pavlov → arunan_bala
Assignee

Comment 7

17 years ago
Comments welcome :)
Assignee

Comment 8

17 years ago
Updated makefile for BMP decoder directory.
Assignee

Comment 9

17 years ago
Posted file nsWBMPDecoder.cpp (obsolete) —
Attachment #108023 - Attachment is obsolete: true
Assignee

Comment 10

17 years ago
Posted file nsWBMPDecoder.h (obsolete) —
Attachment #108024 - Attachment is obsolete: true
Assignee

Comment 11

17 years ago
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.
Reporter

Comment 12

17 years ago
Arunan, my regards :)

Please, request a review via Edit link at your attachment. See
http://bugzilla.mozilla.org/flag-help.html for more information.
>modules\libpr0n\decoders\bmp\Makefile

um. no. firstly, modify Makefile.in, secondly, please make this a patch.
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 pavlov@netscape.com (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.
Assignee

Comment 15

17 years ago
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.
Severity: enhancement → blocker
severity -> enhancement, as it should be
Severity: blocker → enhancement
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
Assignee

Comment 18

17 years ago
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.
>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
Assignee

Comment 20

17 years ago
Just twigged that Makefile is generated :)
The severity change was an accident - apologies.
Attachment #108039 - Attachment is obsolete: true
Assignee

Comment 21

17 years ago
Attachment #108022 - Attachment is obsolete: true
Assignee

Updated

17 years ago
Attachment #108212 - Flags: superreview?(cbiesinger)
Assignee

Updated

17 years ago
Attachment #108213 - Flags: superreview?(cbiesinger)
Assignee

Updated

17 years ago
Attachment #108042 - Flags: superreview?(cbiesinger)
Assignee

Updated

17 years ago
Attachment #108040 - Flags: superreview?(cbiesinger)
Assignee

Comment 22

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

Updated

17 years ago
Attachment #108040 - Flags: superreview?(cbiesinger) → review?(cbiesinger)
Assignee

Updated

17 years ago
Attachment #108042 - Flags: superreview?(cbiesinger) → review?(cbiesinger)
Assignee

Updated

17 years ago
Attachment #108212 - Flags: superreview?(cbiesinger) → review?(cbiesinger)
Assignee

Updated

17 years ago
Attachment #108213 - Flags: superreview?(cbiesinger) → review?(cbiesinger)
Reporter

Comment 23

17 years ago
Nice work, Arunan :))

Can you any idea about bug 35995? If yes, welcome to discussion there.
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?
/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
Assignee

Comment 26

17 years ago
Assignee

Comment 27

17 years ago
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.
Reporter

Comment 28

17 years ago
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! :)
Assignee

Comment 29

17 years ago
Posted file nsWBMPDecoder.cpp (obsolete) —
Adds a cast to remove a warning.
Attachment #108040 - Attachment is obsolete: true
Assignee

Comment 30

17 years ago
Posted file nsWBMPDecoder.h (obsolete) —
Change mCurLine to PRUint32 to remove warnings.
Attachment #108042 - Attachment is obsolete: true
Assignee

Updated

17 years ago
Attachment #109434 - Flags: review?(cbiesinger)
Assignee

Updated

17 years ago
Attachment #109435 - Flags: review?(cbiesinger)
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?
Attachment #109435 - Flags: review?(cbiesinger) → review-
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?)
Attachment #109434 - Flags: review?(cbiesinger) → review-
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 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
(pavlov@netscape.com last I checked) to approve adding WAP BMP support to
imagelib..
Attachment #108212 - Flags: review?(cbiesinger) → review+

Comment 35

17 years ago
It is me now... see bug 185140
I will get back to you shortly...
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
Attachment #108213 - Flags: review?(cbiesinger) → review+
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
Assignee

Comment 38

17 years ago
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.
Assignee

Comment 39

17 years ago
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.
>  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?
Assignee

Comment 41

17 years ago
> 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.
Severity: enhancement → trivial
Reporter

Comment 42

17 years ago
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 :)
Severity: trivial → enhancement
> 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?
Assignee

Comment 44

17 years ago
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.
Assignee

Comment 45

17 years ago
Posted file Zip of test files.
Assignee

Comment 46

17 years ago
Assignee

Comment 47

17 years ago
Posted file nsWBMPDecoder.cpp (obsolete) —
Attachment #109434 - Attachment is obsolete: true
Assignee

Comment 48

17 years ago
Posted file nsWBMPDecoder.h (obsolete) —
Attachment #109435 - Attachment is obsolete: true
Assignee

Updated

17 years ago
Attachment #109786 - Flags: review?(cbiesinger)
Assignee

Updated

17 years ago
Attachment #109788 - Flags: review?(cbiesinger)
Assignee

Comment 49

17 years ago
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>
Reporter

Comment 50

17 years ago
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?
Assignee

Comment 51

17 years ago
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>
Reporter

Comment 52

17 years ago
Nice work :)

Waiting for review.

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
Attachment #109786 - Flags: review?(cbiesinger) → review-
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.
Attachment #109788 - Flags: review?(cbiesinger) → review-
Assignee

Comment 55

17 years ago
Removed string from REQUIRES.
Attachment #108212 - Attachment is obsolete: true
Assignee

Updated

17 years ago
Attachment #110615 - Flags: review?(cbiesinger)
Assignee

Comment 56

17 years ago
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 on attachment 110615 [details]
modules\libpr0n\decoders\wbmp\Makefile.in

better, thanks
Attachment #110615 - Flags: review?(cbiesinger) → review+
Assignee

Comment 58

17 years ago
Attachment #109786 - Attachment is obsolete: true
Assignee

Comment 59

17 years ago
Attachment #109788 - Attachment is obsolete: true
Assignee

Updated

17 years ago
Attachment #110662 - Flags: review?(cbiesinger)
Assignee

Updated

17 years ago
Attachment #110663 - Flags: review?(cbiesinger)

Comment 60

17 years ago
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.
Assignee

Comment 61

17 years ago
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.
Reporter

Comment 62

17 years ago
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.
Assignee

Comment 63

17 years ago
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=.
Assignee

Comment 64

17 years ago
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.
Attachment #108262 - Flags: review?(cls)

Comment 65

17 years ago
sr=jdunn@netscape.com (NOTE: you still need r=
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 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
Attachment #110663 - Flags: review?(cbiesinger) → review+
um. jdunn: you are not listed at http://www.mozilla.org/hacking/reviewers.html
as a super-reviewer?
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)
Attachment #110662 - Flags: review?(cbiesinger) → review+
Assignee

Comment 70

17 years ago
> 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?
if you want :) but I'd say that whoever checks this in can also remove that line.
Assignee

Comment 72

17 years ago
Bug 169304 changed each decoder's Makefile.in by removing the EXPORT_LIBRARY
line.
Attachment #110615 - Attachment is obsolete: true
Assignee

Updated

17 years ago
Attachment #111283 - Flags: review?(cbiesinger)
Comment on attachment 111283 [details]
modules\libpr0n\decoders\wbmp\Makefile.in

looks good
Attachment #111283 - Flags: review?(cbiesinger) → review+
Assignee

Comment 74

17 years ago
Attachment #110663 - Attachment is obsolete: true
Assignee

Comment 75

17 years ago
Posted patch Build changes (obsolete) — Splinter Review
Changes for build (I believe to be reviewed by seawood - cls@seawood.org?)
Attachment #108262 - Attachment is obsolete: true
Assignee

Comment 76

17 years ago
Code changes to make moz show wbmp love.
Attachment #108213 - Attachment is obsolete: true
Assignee

Updated

17 years ago
Attachment #111722 - Flags: review?(cbiesinger)
Assignee

Updated

17 years ago
Attachment #111724 - Flags: review?(cbiesinger)
Assignee

Updated

17 years ago
Attachment #108262 - Flags: review?(cls)
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 - seawood@netscape.com iirc
Attachment #111724 - Flags: review?(cbiesinger) → review+
Assignee

Updated

17 years ago
Attachment #111723 - Flags: review?(seawood)
Comment on attachment 111723 [details] [diff] [review]
Build changes

I'll r= the build changes after pavlov has given module owner approval.
Attachment #111723 - Flags: superreview?(seawood)
Attachment #111723 - Flags: review?(seawood)
Attachment #111723 - Flags: review?(pavlov)
Attachment #111722 - Flags: review?(cbiesinger) → review+

Comment 79

17 years ago
Comment on attachment 111723 [details] [diff] [review]
Build changes

yeah sure
Attachment #111723 - Flags: review?(pavlov) → review+
Comment on attachment 111723 [details] [diff] [review]
Build changes

r=cls
Attachment #111723 - Flags: superreview?(seawood) → superreview+
Assignee

Comment 81

17 years ago
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 :)
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.
Assignee

Updated

17 years ago
Attachment #111283 - Flags: superreview?(tor)
Assignee

Updated

17 years ago
Attachment #111722 - Flags: superreview?(tor)
Assignee

Updated

17 years ago
Attachment #111724 - Flags: superreview?(tor)
Assignee

Comment 83

17 years ago
Removed the NS_INIT_ISUPPORTS() line for the super reviewer.
Attachment #110662 - Attachment is obsolete: true
Assignee

Updated

17 years ago
Attachment #112275 - Flags: superreview?(tor)
Attachment #112275 - Flags: review?(cbiesinger)
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
Attachment #112275 - Flags: review?(cbiesinger) → review+
Assignee

Comment 85

17 years ago
> 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=.
Severity: enhancement → critical
Reporter

Comment 86

17 years ago
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.
Severity: critical → enhancement

Comment 87

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

Comment 88

17 years ago
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.
Reporter

Comment 89

16 years ago
Hope to have this in 1.4 alpha.
Flags: blocking1.4a?
blocking1.4a- because we wouldn't block the release for this change. 
Flags: blocking1.4a? → blocking1.4a-
Assignee

Comment 91

16 years ago
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 :).
> 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

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

Comment 94

15 years ago
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.
Assignee

Comment 95

15 years ago
I would be happy to get this working with current CVS if wanted.
Assignee

Updated

15 years ago
Attachment #145691 - Attachment description: Fixes a bug in the multi-byte integer decoder → modules\libpr0n\decoders\wbmp\nsWBMPDecoder.cpp Fixes a bug in the multi-byte integer decoder
Attachment #145691 - Flags: review?(cbiesinger)
Assignee

Updated

15 years ago
Status: NEW → ASSIGNED
Assignee

Updated

15 years ago
Attachment #111723 - Attachment is obsolete: true
Assignee

Updated

15 years ago
Attachment #145694 - Flags: review?(cbiesinger)
Assignee

Comment 99

15 years ago
Posted patch XPFE changesSplinter Review
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...".
Assignee

Comment 100

15 years ago
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 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
Attachment #145695 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #145694 - Flags: review?(cbiesinger) → review+
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.
Attachment #145691 - Flags: review?(cbiesinger) → review+
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.
Attachment #145695 - Flags: review?(neil.parkwaycc.co.uk) → review?(bsmedberg)
Assignee

Comment 104

15 years ago
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?
> 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
Assignee

Comment 106

15 years ago
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.
Attachment #145691 - Attachment is obsolete: true
Assignee

Updated

15 years ago
Attachment #145740 - Flags: review?(cbiesinger)
Attachment #145740 - Flags: review?(cbiesinger) → review+

Updated

15 years ago
Attachment #145693 - Flags: review?(bsmedberg) → review+
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.
Attachment #145695 - Flags: review?(bsmedberg) → review+
Assignee

Comment 108

15 years ago
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?
Assignee

Updated

15 years ago
Attachment #145740 - Flags: superreview?(tor)
Assignee

Updated

15 years ago
Attachment #145694 - Flags: superreview?(tor)
Assignee

Updated

15 years ago
Attachment #112275 - Flags: superreview?(tor)
Assignee

Updated

15 years ago
Attachment #111724 - Flags: superreview?(tor)

Comment 109

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

Comment 110

13 years ago
This really probably needs to be updated after the Cairo changes

Updated

12 years ago
Attachment #111283 - Flags: superreview?(tor)

Updated

12 years ago
Attachment #111722 - Flags: superreview?(tor)

Updated

12 years ago
Attachment #145694 - Flags: superreview?(tor)

Updated

12 years ago
Attachment #145740 - Flags: superreview?(tor)

Comment 111

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

Comment 112

10 years ago
What's going on here? Is it going to be included or not?
QA Contact: tpreston → imagelib
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WONTFIX
Bug 847310 kinda changed this to DUPLICATE FIXED.
You need to log in before you can comment on or make changes to this bug.