Add some more features to nsRegion

RESOLVED FIXED

Status

defect
RESOLVED FIXED
14 years ago
11 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Posted patch fixSplinter Review
Attachment #204014 - Flags: superreview?(bzbarsky)
Attachment #204014 - Flags: review?(Dainis_Jonitis)

Comment 2

14 years ago
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 
(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

14 years ago
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.
Attachment #204014 - Flags: review?(Dainis_Jonitis) → review+
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?
Attachment #204014 - Flags: superreview?(bzbarsky) → superreview+
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.
Shouldn't this bug be marked resolved->fixed?
yes
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.