Three BMP/ICO decoder tweaks

RESOLVED FIXED in Firefox 44

Status

()

Core
ImageLib
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
I have a couple of BMP decoder tweaks.
(Assignee)

Comment 1

3 years ago
Actually, I have three, and some are in the ICO decoder.
Summary: Two BMP decoder tweaks → Three BMP/ICO decoder tweaks
(Assignee)

Comment 2

3 years ago
Created attachment 8674633 [details] [diff] [review]
(part 1) - Don't set mBPP twice in nsICODecoder.cpp

Seth's "XXX" comment is correct.
Attachment #8674633 - Flags: review?(seth)
(Assignee)

Comment 3

3 years ago
Created attachment 8674635 [details] [diff] [review]
(part 2) - Streamline nsBMPDecoder's getters

- GetBitsPerPixel() and GetWidth() are no longer used.

- GetHeight() is now only used within nsBMPDecoder and can be renamed and
  inlined into the header.

- GetImageData() can be inlined into the header.
Attachment #8674635 - Flags: review?(seth)
(Assignee)

Comment 4

3 years ago
Created attachment 8674638 [details] [diff] [review]
(part 3) - Deconvolute nsICODecoder's handling of endianness

nsICODecoder's reading and writing of little-endian values can be simplified
greatly.

Also, ReadBPP() was highly dodgy: BMP's bpp field is 16-bit
but ReadBPP() read it as if it's 32-bit. I think this currently works because
the bpp field is followed by the 32-bit compression field which is usually 0
for BMPs within ICOs!
Attachment #8674638 - Flags: review?(seth)
Comment on attachment 8674633 [details] [diff] [review]
(part 1) - Don't set mBPP twice in nsICODecoder.cpp

Review of attachment 8674633 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.
Attachment #8674633 - Flags: review?(seth) → review+
Comment on attachment 8674635 [details] [diff] [review]
(part 2) - Streamline nsBMPDecoder's getters

Review of attachment 8674635 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!
Attachment #8674635 - Flags: review?(seth) → review+
Comment on attachment 8674638 [details] [diff] [review]
(part 3) - Deconvolute nsICODecoder's handling of endianness

Review of attachment 8674638 [details] [diff] [review]:
-----------------------------------------------------------------

Much cleaner!
Attachment #8674638 - Flags: review?(seth) → review+
https://hg.mozilla.org/mozilla-central/rev/a4c60957e505
https://hg.mozilla.org/mozilla-central/rev/1c6abaa798f9
https://hg.mozilla.org/mozilla-central/rev/6928d7087fed
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.