User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20031007 Firebird/0.7 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20031007 Firebird/0.7 1. The nsRegion uses the nsRectFast for faster inline operations than original nsRect has. But that implementation detail should not be visible for callers - they should work with nsRect. 2. The nsRegion operations (Or, Xor and Sub) between two rectangles should not create the temporary region. Some speed improvement. 3. As nsRegion can be used alone (and not only as a part of nsIRegion implementation), there is no point to prohibit the copying or one region by copy constructor or = operator. Previosly used function Copy () is now private - copy constructor and = operator should be used instead to make it more similar to nsRect. Create = operator to allow assign nsRect to nsRegion. 4. Whitespace cleanup. Other developers which touched the code introduced some formatting which does not mach the rest of the code. I am changing it back to my style :) Reproducible: Always Steps to Reproduce:
I verified it only on Win32 with MS VC 6.0. Mike, can you please check if it works with OS/2 and Linux?
Forgot to mention that: 5. GetBounds () now returns 'const nsRect&' instead of nsRect. 6. I removed the NS_GFX modifier for nsRegion::RgnRect class (which should be private), but added to nsRegionRectIterator. Is it ok?
Oh, I forgot to remove one dummy garbage line in nsRegion::Or (aRegion, aRect) + nsRectFast zzz; That should go away! Sorry
formally confirming because there is a patch
Status: UNCONFIRMED → NEW
Ever confirmed: true
Created attachment 137460 [details] [diff] [review] diff -u with correct Index: lines Fixed up version of the patch that will apply cleanly. Includes removal of the zzz variable. Passes basic tests on linux.
Comment on attachment 137362 [details] [diff] [review] diff -u I think the methods that you define as inline should be marked inline where they are declared in the class. Also, please add a comment at the declaration of nsRectFast to remind people that they can *never* add fields to this class because of the casts that are used from nsRect to nsRectFast. Otherwise, looks good.
Comment on attachment 137362 [details] [diff] [review] diff -u marking r+sr on the assumption that you address those comments
Created attachment 137667 [details] [diff] [review] diff -u Addresses Roc`s comments: 1. Added back 'inline' in the declarations of functions. Even if ISO C++ standard says that it is enough if either definition or declaration has inline keyword, it is better to have it for both just for consistency. 2. Added comment to preserve the data part of nsRectFast identical to nsRect, to make sure that casts work as expected. BTW is it possible in C++ to create default constructor of child class that does not call the constructor of parent class? nsRectFast () actually does not have to initialize x, y, width and height to 0 by calling default nsRect () constructor. Thanks all for reviewing and testing this!
> BTW is it possible in C++ to create default constructor of child class that > does not call the constructor of parent class? I don't believe so. Thanks!
Robert, can you please check in this code. David Baron in bug 230118 was fixing things that would not be required with new nsRegion.
Ah, OK. I can check this in. Taking for checkin.
Assignee: general → roc
Priority: -- → P1
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
The nsContainerFrame.cpp change broken AIX, HP-UX, and IRIX native compilers: http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey-Ports
I checked in a fix/workaround for this, using a temporary for the nsRegion.
You need to log in before you can comment on or make changes to this bug.