317514 - Add some more features to nsRegion

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect)

Description

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

For my frame display list work, I need some new features in nsRegion:

-- fast Contains and Intersects methods to check whether a region contains or intersects a rectangle

-- efficient "loose" subtraction of a rectangle from a region that is not allowed to make a region more complex. Basically, we remove any rectangle from the region's rectangle list that is contained by the rectangle parameter.

-- loose subtraction of one region from another by applying the above method to each rectangle in the region-to-subtract.

Comment 1

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

Attachment: fix

Comment 2

[Dainis Jonitis]

1. Please replace the MIN_INT32 and MAX_INT32 with PR_INT32_MIN and PR_INT32_MAX. Those were added to nsprpub after the initial checkin of nsRegion.

2. Make CanSimpleSubtract () const method

3. Should not SimpleSubtract () methods update the mBoundRect? If no, then at least add comment in nsRegion.h 

Comment 3

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

(In reply to comment #2)
> 1. Please replace the MIN_INT32 and MAX_INT32 with PR_INT32_MIN and
> PR_INT32_MAX. Those were added to nsprpub after the initial checkin of
> nsRegion.

You mean everywhere in nsRegion?

> 2. Make CanSimpleSubtract () const method

sure

> 3. Should not SimpleSubtract () methods update the mBoundRect? If no, then at
> least add comment in nsRegion.h 

Yes it should. What's the best way to do that, call Optimize()?

Comment 4

[Dainis Jonitis]

Comment on attachment 204014 [details] [diff] [review]
fix

> You mean everywhere in nsRegion?

Yes. No need for homegrown macros.

> Yes it should. What's the best way to do that, call Optimize()?

Yes, Optimize () will update the bounds rectangle. It will have some additional overhead of trying to combine adjacent rectangles, but for first version it should be good enough. Later we can add some special cases (e.g. if rectangle is strictly within bounds rectangle then when removed it will not change the bounds and we do not need to traverse entire rectangle chain). If you do not mind I can think about possible optimizations :)

r+ with above problems fixed.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 204014 [details] [diff] [review]
fix

>Index: gfx/public/nsRegion.h
>+   * Efficiently try to remove a region from this region. This method is
>+   * allowed to remove any sub-area of this region up to the bounds of the
>+   * subtracted region.

I assume that here "bounds" doesn't mean "bounding rect", right?

>+PRBool nsRegion::Intersects (const nsRect& aRect) const
>+  if (!IsComplex())
>+    return mBoundRect.Intersects (aRect);

Is that really needed?  It doesn't seem like much of an optimization to me...

>+PRBool nsRegion::CanSimpleSubtract (const nsRect& aRect)
>+    const RgnRect* next = r->next;
>+    if (aRect.Contains(*r))
>+      return PR_TRUE;
>+    r = next;

Why do you need the |next| thing?  Just to look more like SimpleSubtract?  Either way's ok, I guess...

Document that SimpleSubtract should not be passed one of this region's rects?  And that foo->SimpleSubtract(foo) for foo a region fails?

Comment 6

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

checked in, with comments taken into account. 

> I assume that here "bounds" doesn't mean "bounding rect", right?

Reworded...

> Is that really needed?  It doesn't seem like much of an optimization to me...

Removed

> Why do you need the |next| thing?  Just to look more like SimpleSubtract? 

Oversight. Actually I removed CanSimpleSubtract because I don't use it.

> Document that SimpleSubtract should not be passed one of this region's rects?
> And that foo->SimpleSubtract(foo) for foo a region fails?

Added special case code for region.SimpleSubtract(region) to avoid that problem. Added simple code to SimpleSubtract(aRect) to avoid the problem there.

Comment 7

[Martijn Wargers (dead)]

Shouldn't this bug be marked resolved->fixed?

Comment 8

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

yes