Status

Core Graveyard
GFX
P1
normal
RESOLVED FIXED
15 years ago
9 years ago

People

(Reporter: Dainis Jonitis, Assigned: roc)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

15 years ago
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:
(Reporter)

Comment 1

15 years ago
Created attachment 137361 [details] [diff] [review]
diff -uw
(Reporter)

Comment 2

15 years ago
Created attachment 137362 [details] [diff] [review]
diff -u
(Reporter)

Updated

15 years ago
Attachment #137362 - Flags: superreview?(roc)
Attachment #137362 - Flags: review?(mkaply)
(Reporter)

Comment 3

15 years ago
I verified it only on Win32 with MS VC 6.0.

Mike, can you please check if it works with OS/2 and Linux?
(Reporter)

Comment 4

15 years ago
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?
(Reporter)

Comment 5

15 years ago
Oh, I forgot to remove one dummy garbage line in nsRegion::Or (aRegion, aRect)

+    nsRectFast zzz;

That should go away! Sorry

Comment 6

15 years ago
formally confirming because there is a patch
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 7

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

Updated

15 years ago
Attachment #137362 - Attachment is obsolete: true
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
Attachment #137362 - Flags: superreview?(roc)
Attachment #137362 - Flags: superreview+
Attachment #137362 - Flags: review?(mkaply)
Attachment #137362 - Flags: review+
(Reporter)

Comment 10

15 years ago
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!
(Reporter)

Updated

15 years ago
Attachment #137361 - Attachment is obsolete: true
> 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!
(Reporter)

Comment 12

15 years ago
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
checked in.
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.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.