Cleanup nsIImage

RESOLVED FIXED in mozilla1.3beta

Status

Core Graveyard
Image: Painting
P1
minor
RESOLVED FIXED
15 years ago
8 years ago

People

(Reporter: paper, Assigned: paper)

Tracking

Trunk
mozilla1.3beta

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 15 obsolete attachments)

25.91 KB, application/x-zip-compressed
paper
: review+
Details
(Assignee)

Description

15 years ago
I've CC'd all the module owners and peers of mozilla/gfx/* since this effects
them all.  Please feel free to unCC yourself if you don't care. :)
----------------------
Some nsIImage stuff is not being used at all and others are redundant.  It's not
needed and confuses newbies as well as people trying to write for new platforms.

Remove the following functions from nsIImage:

GetAlphaWidth()
GetAlphaHeight()

GetAlphaLevel()
SetAlphaLevel(..)

GetNaturalWidth()
SetNaturalWidth(..)
GetNaturalHeight()
SetNaturalHeight(..)

SetDecodedRect  There's only one outside call to SetDecodedRect, and one 
GetDecodedX1    outside call to GetDecodedY2.  Neither of these are needed
GetDecodedX2    after a simple code change.
GetDecodedY1    (Someday we'll want to get rid of the variables as well, and
GetDecodedY2     store what areas of the image are avaiable in layout, where
                 Stuart says it should be anyway)

IsOptimized()
----------------------
Remove variables from nsImage*:

mAlphaLevel     only a few platforms have this variable

mNaturalHeight  See Bug 116649
mNaturalWidth

mAlphaWidth     These two are always the same as the image width and height.
mAlphaHeight    Even if we did support different mAlphaWidth/Heights, wouldn't
                a start location of the alpha also be needed?  Nuke it :)
----------------------
One might say "Why remove all these, they might be usefull".. well, they've been 
in the code for over 3 years, and they *still* aren't being used.  Most likely
they will never be used.  And if they are needed one day, then the person
needing them can add them back in easily.  In the mean time, we'll have a 
cleaner, smaller nsIImage and nsImage*.

If anyone has a good reason any of the above can't be removed, or have spotted
an additional one that can be removed, post a comment explaining :)
(Assignee)

Comment 1

15 years ago
Two others to remove:

DuplicateImage()
MoveAlphaMask()

These aren't in nsIImage, but several nsImage* have them, and they are blank..
so many platforms probably have it because they used nsImageGTK as a template.
(Assignee)

Comment 2

15 years ago
going CC happy here
(Assignee)

Comment 3

15 years ago
Created attachment 107440 [details] [diff] [review]
XP patch

for gfxImageFrame.cpp:
Chunk #2: Code has been moved to nsImage*::ImageUpdated(..)

for imgContainerGIF.cpp:
Chunk #1: SetDecodedRect removed. About 10 lines later, BlackenFrame is called,
which calls ImageUpdated.  ImageUpdated() now dows what SetDecodedRect did.. so
everything should be okay.
(Assignee)

Comment 4

15 years ago
Created attachment 107441 [details] [diff] [review]
The BeOS Patch

arougthopher@lizardland.net, mozilla@switkin.com, could one of you (or both!)
see if this patch compiles and r=?  You'll also have to apply Attachment 107440 [details] [diff]
(Assignee)

Comment 5

15 years ago
Created attachment 107442 [details] [diff] [review]
The GTK Patch

Need someone to see if this compiles with Attachment 107440 [details] [diff] :)
and a r/sr :)
(Assignee)

Comment 6

15 years ago
Created attachment 107443 [details] [diff] [review]
The Mac Patch
(Assignee)

Comment 7

15 years ago
Created attachment 107444 [details] [diff] [review]
The OS2 Patch

mkaply, could you compile this patch at Attachement 17440, see if images still
display, and r=? :)
(Assignee)

Comment 8

15 years ago
Created attachment 107445 [details] [diff] [review]
The Photon Patch

dinglis@qnx.com, briane@qnx.com, could one of you try this patch and Patch
107440, see if it compiles, and r=?
(Assignee)

Comment 9

15 years ago
Created attachment 107446 [details] [diff] [review]
The Qt Patch
(Assignee)

Comment 10

15 years ago
Created attachment 107447 [details] [diff] [review]
The Windows Patch
(Assignee)

Comment 11

15 years ago
Created attachment 107449 [details] [diff] [review]
The XLib Patch!
(Assignee)

Comment 12

15 years ago
Created attachment 107451 [details] [diff] [review]
The Windows Patch

This one actually draws image correctly ;)
Attachment #107447 - Attachment is obsolete: true
(Assignee)

Comment 13

15 years ago
Created attachment 107452 [details] [diff] [review]
The GTK Patch

..and this one actually compiles now :)
Attachment #107442 - Attachment is obsolete: true
Comment on attachment 107452 [details] [diff] [review]
The GTK Patch

+	// Assumed: [...]
you have a tab here, please fix that. (though no need to attach a new patch)
Comment on attachment 107444 [details] [diff] [review]
The OS2 Patch

r=pedemont (in place of mkaply)
Attachment #107444 - Flags: review+
(Assignee)

Comment 16

15 years ago
Comment on attachment 107440 [details] [diff] [review]
XP patch

Rod, could you review?
Thanks:)
Attachment #107440 - Flags: review?(rods)
(Assignee)

Comment 17

15 years ago
Comment on attachment 107451 [details] [diff] [review]
The Windows Patch

..this one too, please
Attachment #107451 - Flags: review?(rods)

Comment 18

15 years ago
Comment on attachment 107441 [details] [diff] [review]
The BeOS Patch

Seems to be ok to me.
r=arougthopher
Attachment #107441 - Flags: review+

Comment 19

15 years ago
I have some questions... 
are we not using the decoded sizes anymore?   I see where
we set them in the GIF decoder... and where we use them in nsImage.  I know we
use to use them to keep from painting garbage when the entire image was not read
in yet.

Also.. just because the Init is currently only called once... it may be useful..
and thats the purpose of init.. is to reset the values of the object independent
of the constructor.. right.   I
I noticed that the init now assumes the constructor has set the values.. isnt that
wrong?

I noticed where you are using the header information.. instead of the object
variables.. also.. you are assuming that the alpha image is the same size as the
main image... I am not conviced these two points will always hold true
Also.. the mBHead->biHeight can be negative.. right.  So wouldnt the following
have a problem? 

example 
-            MONOBITMAPINFO  bmi(mAlphaWidth, mAlphaHeight);
+            MONOBITMAPINFO  bmi(mBHead->biWidth, mBHead->biHeight);

why are you assuming GetIsRowOrderTopToBottom() is always false and we don't
need that variable.  It is legal and used alot in windows to reverse that order
(hence the negative mBHead->biWidth..) 

Also.. I have a patch that got it review and is getting a Code review that
cleans up some of these issues.. its for the GIF 8 bit patch (143046).  Can this
patch wait for that to get in.. since I have changed alot of stuff in nsImage.  
(Assignee)

Comment 20

15 years ago
>are we not using the decoded sizes anymore?   I see where
>we set them in the GIF decoder... and where we use them in nsImage.  

It doesn't look like any of the Get's were ever used aside from gfxImageFrame. 
Perhaps the GIF decoder needed to set the decoded rect before, but now it's
totally useless.  Anything that changes the image data should be calling
ImageUpdated(..).  It's the perfect place to set the decoded area (since it's
being passed the area that's been updated).  gfxImageFrame::SetImageData already
calls ImageUpdated for the caller, so any code using SetImageData to build an
image is already updating the decoded area.


>I know we use to use them to keep from painting garbage when the entire image 
>was not read in yet.

Yes, internally we need the decoded sizes, but we don't need to use
GetDecoded[XY][12] to get them in there, we just use the variables.


>Also.. just because the Init is currently only called once... it may be useful..
>and thats the purpose of init.. is to reset the values of the object independent
>of the constructor.. right.   I
>I noticed that the init now assumes the constructor has set the values.. isnt that
>wrong?

It's only wrong if Init is being called more than once, which it never ever is.
If Init is being called only once (which it only can be), why set variables to
something that they are already being set to in the constructor?  This is a
waste of CPU cycles and bloat of code in my mind. 


>I noticed where you are using the header information.. instead of the object
>variables.. also.. you are assuming that the alpha image is the same size as the
>main image... I am not conviced these two points will always hold true
>Also.. the mBHead->biHeight can be negative.. right.  So wouldnt the following
>have a problem? 

biHeight is assumed to be positive in all other code in nsImageWin.  If that
assumption wasn't the case, then the code would have broken so many times over
by now.  There are for loops that go from 0 to biHeight that would have broken,
and passed in heights are being compared as if biHeight was positive.  We
already assume alpha width/height is the same as the image in this line:

366  ALPHA32BITMAPINFO bmi(mBHead->biWidth, mBHead->biHeight);

And as I've said in Comment #0, if we wanted to start supporting alpha layers
that aren't the same size as the image, we'd have to start storing the location
where the alpha layer starts within the image, and a lot of code would have to
be rewritten.  I'm not saying that smaller alpha layers shouldn't be done.. I'm
just saying they aren't done, and those variables/functions aren't needed until
someone writes such a patch (if they'll need them at all).  Realistically, I
don't see anyone creating such a patch.


>why are you assuming GetIsRowOrderTopToBottom() is always false and we don't
>need that variable.  It is legal and used alot in windows to reverse that order
>(hence the negative mBHead->biWidth..) 

Again, like biHeight, the existing code already assumes biWidth is positive. 
Windows stores images bottom to top.  Microsoft isn't going to change this now.
 Window's GDI is always going to store images bottom to top.  Why would we want
to store one image top to bottom, and the next bottom to top anyway?  Isn't
consistency good?  All images on a single platform are stored in the same direction.


>Also.. I have a patch that got it review and is getting a Code review that
>cleans up some of these issues.. its for the GIF 8 bit patch (143046).  Can this
>patch wait for that to get in.. since I have changed alot of stuff in nsImage.  

This doesn't mean that both patches can't be worked on, reviewed, etc
simultaineously.

Comment 21

15 years ago
1.)  I agree with the logic you have about using the header.  Just makes things
easier to maintain.  

2.)  I agree with the logic about the negative height.. but I think we have to
comment that negative height is not supported in our code (not that it would
have worked before.. but it was trying to be supported).  I assume you did not
take out the entire method because it is used somewhere... right?

3.)  The decoded size is ok.. make sure that Pavlov or Saari know this since it
comes down to there responsibility for that.

4.)  My only uncomfortableness is the init.  I just believe an init.. should
initialize the object.. otherwise.. why have an init.  I know we only call it
once.. but someone might use it again in the future.. and then something will
break.  It bloat is what your concerned about.. then maybee the init should do
everything.  I know I would be happier if init still... initited the class.

I will r it for you.. what ever we decide on 4.. it might just take me a day to
think it thru.  I will ask for opionions, and take a look how exacty nsImage
uses this init or should use it.
(Assignee)

Updated

15 years ago
Attachment #107451 - Flags: review?(rods)

Comment 22

15 years ago
Please dcone review this, it is his ownership area.
(Assignee)

Comment 23

15 years ago
Created attachment 108076 [details] [diff] [review]
Windows Patch

Well, I'd rather be safe than sorry too, so I added an initialized variable,
set it to false in the constructor, abort the init if it's already true, and
set it to true at the end of init.

With regards to the variable initializing in the constructor, the reason it's
done is usually the compiler can optimize it into one memcpy, which is faster
than multiple sets.

Thanks for the review offer.  I very much appreciate it.
Attachment #107451 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #107440 - Flags: review?(rods) → review?(dcone)
(Assignee)

Updated

15 years ago
Attachment #108076 - Flags: review?(dcone)

Comment 24

15 years ago
Comment on attachment 108076 [details] [diff] [review]
Windows Patch

r=dcone.  I was making sure how we use our init.  I think a use only once init
is fine.  I am checking in.. soon I hope a patch that touches some of what you
did, fixes some stuff.. it should not effect this patch except I doot out the
CleanUpDIBSection completely.. and the associated member.  Just a heads up.. my
sr should be any day.
Attachment #108076 - Flags: review?(dcone) → review+
(Assignee)

Comment 25

15 years ago
Thanks for the heads up and review, dcone.  The conflicts shouldn't be to bad.

Could you also review the XP patch (Attachment 107440 [details] [diff])?  (if not, I can get
someone else to do it)

Comment 26

15 years ago
Comment on attachment 107440 [details] [diff] [review]
XP patch

r=dcone on XP
Attachment #107440 - Flags: review?(dcone) → review+
Comment on attachment 107449 [details] [diff] [review]
The XLib Patch!

guess gisburn would be a good person to review this
Attachment #107449 - Flags: review?(Roland.Mainz)
(Assignee)

Comment 28

15 years ago
Updating some flags.
Reviews for other patches are in progress, I'm just too lazy to mark them r?
(plus it creates bugspam)
Severity: normal → minor
Priority: -- → P1
Target Milestone: --- → mozilla1.3beta
(Assignee)

Comment 29

15 years ago
biesi, timeless, or anyone with Qt.. now that Qt actually compiles, could you
try the Qt Patch along with the XP Patch (both are needed) and see if there's
any compile errors?

If you get patching conflicts, just say so and I'll refresh the patch(es).

Oh, if it does compile, you could r+ it and save tor some time :)
Comment on attachment 107446 [details] [diff] [review]
The Qt Patch

patching file nsImageQT.cpp
Hunk #1 FAILED at 53.
Hunk #2 succeeded at 108 (offset 1 line).
Hunk #3 FAILED at 135.
Hunk #4 succeeded at 203 (offset 2 lines).
Hunk #5 succeeded at 247 (offset 2 lines).
Hunk #6 succeeded at 434 (offset 1 line).
Hunk #7 succeeded at 447 with fuzz 2 (offset 1 line).
2 out of 7 hunks FAILED -- saving rejects to file nsImageQT.cpp.rej
(Assignee)

Comment 31

15 years ago
Created attachment 109553 [details] [diff] [review]
Qt Patch

Updated Qt Diff
Attachment #107446 - Attachment is obsolete: true
Comment on attachment 109553 [details] [diff] [review]
Qt Patch

+  virtual PRBool      GetIsRowOrderTopToBottom() {return PR_TRUE};

if the ; is moved before the } this even compiles :)
and images still render.

timeless being the port owner, he should probably review this; requesting
r=timeless.
Attachment #109553 - Flags: review?(timeless)

Comment 33

15 years ago
Comment on attachment 109553 [details] [diff] [review]
Qt Patch

it looks fine to me with biesi's stipulated change.
Attachment #109553 - Flags: review?(timeless) → review+
(Assignee)

Comment 34

15 years ago
Created attachment 109910 [details] [diff] [review]
GTK Patch

Updated to 2002-12-21
Attachment #107452 - Attachment is obsolete: true
(Assignee)

Comment 35

15 years ago
Created attachment 109911 [details] [diff] [review]
Mac Patch

Updated to 2002-12-21
Attachment #107443 - Attachment is obsolete: true

Comment 36

15 years ago
Comment on attachment 109910 [details] [diff] [review]
GTK Patch

The switch(aMaskRequirements) needs a default: section to avoid a compiler
warning about unhandled enums.
That and r=tor.
Attachment #109910 - Flags: review+

Comment 37

15 years ago
Comment on attachment 107445 [details] [diff] [review]
The Photon Patch

Same comment regarding the switch statement.  r=tor
Attachment #107445 - Flags: review+

Comment 38

15 years ago
Comment on attachment 109911 [details] [diff] [review]
Mac Patch

Same comment regarding the switch statement.  r=tor
Attachment #109911 - Flags: review+

Comment 39

15 years ago
Comment on attachment 107449 [details] [diff] [review]
The XLib Patch!

Same comment regarding the switch statement.  r=tor
Attachment #107449 - Flags: review?(Roland.Mainz) → review+

Comment 40

15 years ago
*** Bug 116649 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 41

15 years ago
roc, my would be SR ginuea pig, is away until the 13th.  I have no idea who else
to ask for a SR.  I'm guessing some of these patches only need a RS.  If anyone
has alternate SR suggestions, please comment, otherwise this bug will patiently
wait a couple more weeks.
(Assignee)

Comment 42

15 years ago
Created attachment 111267 [details]
9 Patches in 1 Zip

All 9 patches unbitrotted.  No other changes, unless you include those switch
defaults that tor requested.
Attachment #107440 - Attachment is obsolete: true
Attachment #107441 - Attachment is obsolete: true
Attachment #107444 - Attachment is obsolete: true
Attachment #107445 - Attachment is obsolete: true
Attachment #107449 - Attachment is obsolete: true
Attachment #108076 - Attachment is obsolete: true
Attachment #109553 - Attachment is obsolete: true
Attachment #109910 - Attachment is obsolete: true
Attachment #109911 - Attachment is obsolete: true
(Assignee)

Comment 43

15 years ago
Comment on attachment 111267 [details]
9 Patches in 1 Zip

transferring r+
See other patches for people who reviewed them
Attachment #111267 - Attachment is patch: true
Attachment #111267 - Attachment mime type: application/x-zip-compressed → text/plain
Attachment #111267 - Flags: superreview?(roc+moz)
Attachment #111267 - Flags: review+
(Assignee)

Updated

15 years ago
Attachment #111267 - Attachment is patch: false
Attachment #111267 - Attachment mime type: text/plain → application/x-zip-compressed
(Assignee)

Comment 44

15 years ago
roc, do you think you'll have time to SR before the freeze?  If not, I'll mark
this as a 1.4a target milestone.
I will sr it before the freeze.
Comment on attachment 111267 [details]
9 Patches in 1 Zip

sr=roc+moz

just one question ... did you add the mInitialized check to nsImageWin just so
that any multiple-initialization bugs will be detected there?
Attachment #111267 - Flags: superreview?(roc+moz) → superreview+
(Assignee)

Comment 47

15 years ago
Thanks for the SR.
The mInitialized check was added to appease dcone. :)  No one on the other
platforms raised concerns about nsIImage being initialized twice (which I still
maintain is impossible since gfxIImageFrame::Init is the only one who creates a
nsIImage, and gfxIImageFrame has a mInitialized check for itself before the
nsIImage)


I'll be checking this in after OS2VACPP goes green, when there's a quiet moment
(Perhaps late Sunday night)
this was checked in yesterday, should this bug be marked fixed?
(Assignee)

Comment 49

15 years ago
Yes, I fell asleep before marking the bug fixed :P
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Updated

8 years ago
Component: Image: Painting → Image: Painting
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.