228378 - Clean up nsRegion a bit

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect, P1)

Description

[Dainis Jonitis]

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:

Comment 1

[Dainis Jonitis]

Attachment: diff -uw

Comment 2

[Dainis Jonitis]

Attachment: diff -u

Comment 3

[Dainis Jonitis]

I verified it only on Win32 with MS VC 6.0.

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

Comment 4

[Dainis Jonitis]

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?

Comment 5

[Dainis Jonitis]

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

+    nsRectFast zzz;

That should go away! Sorry

Comment 6

[Bernard Alleysson]

formally confirming because there is a patch

Comment 7

[tor]

Attachment: 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 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 137362 [details] [diff] [review]
diff -u

marking r+sr on the assumption that you address those comments

Comment 10

[Dainis Jonitis]

Attachment: 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!

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> 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!

Comment 12

[Dainis Jonitis]

Robert, can you please check in this code.
David Baron in bug 230118 was fixing things that would not be required with new
nsRegion.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ah, OK. I can check this in. Taking for checkin.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The nsContainerFrame.cpp change broken AIX, HP-UX, and IRIX native compilers:

http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey-Ports

Comment 16

[Brian Ryner (not reading)]

I checked in a fix/workaround for this, using a temporary for the nsRegion.