Closed Bug 445269 Opened 12 years ago Closed 12 years ago

Change nsRect to nsIntRect in nsSVGFilters.cpp

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

Details

Attachments

(1 file)

Attached patch fixSplinter Review
This is just cleanup to change nsRect to nsIntRect in nsSVGFilters.cpp to make it clear these are pixel rects, not appunit rects. Since nsIntRect is currently typedefed to nsRect this won't change any behaviour.
Attachment #329607 - Flags: superreview?(mats.palmgren)
Attachment #329607 - Flags: review?(longsonr)
Comment on attachment 329607 [details] [diff] [review]
fix

> static void
> ClipComputationRectToSurface(nsSVGFilterInstance* aInstance,
>                              nsIntRect* aDataRect)
> {
>   aDataRect->IntersectRect(*aDataRect,
>-          nsRect(nsPoint(0, 0), aInstance->GetSurfaceRect().Size()));
>+          nsIntRect(nsPoint(0, 0), aInstance->GetSurfaceRect().Size()));

nsIntRect only has this constructor by dint of it being a typedef of nsRect. If NS_COORD_IS_FLOAT is ever defined then this won't compile. Seems easy to use GetSufraceWidth and GetSurfaceHeight to avoid.

>   NS_IMETHOD FrameChanged(imgIContainer *aContainer, gfxIImageFrame *newframe,
>-                          nsRect * dirtyRect);
>+                          nsIntRect * dirtyRect);

Nit: Even though we're all over the place regarding whether its type* x or type *x or type * x which is not your fault. We might at least try to be consistent within a single function.

> NS_IMETHODIMP
> nsSVGFEImageElement::FrameChanged(imgIContainer *aContainer,
>                                   gfxIImageFrame *newframe,
>-                                  nsRect * dirtyRect)
>+                                  nsIntRect * dirtyRect)

ditto.

...

One other nit which I didn't notice before, you could use some spaces after the commas in this assertion.
 NS_ASSERTION(nsIntRect(0,0,aDest->mImage->Width(),aDest->mImage->Height()).Contains(aDataRect),
               "aDataRect out of bounds");

Note there are two occurrances of this line in nsSVGFilters.cpp
Attachment #329607 - Flags: review?(longsonr) → review+
Comment on attachment 329607 [details] [diff] [review]
fix

sr=mats
Attachment #329607 - Flags: superreview?(mats.palmgren) → superreview+
(In reply to comment #1)
> (From update of attachment 329607 [details] [diff] [review])
> > static void
> > ClipComputationRectToSurface(nsSVGFilterInstance* aInstance,
> >                              nsIntRect* aDataRect)
> > {
> >   aDataRect->IntersectRect(*aDataRect,
> >-          nsRect(nsPoint(0, 0), aInstance->GetSurfaceRect().Size()));
> >+          nsIntRect(nsPoint(0, 0), aInstance->GetSurfaceRect().Size()));
> 
> nsIntRect only has this constructor by dint of it being a typedef of nsRect. If
> NS_COORD_IS_FLOAT is ever defined then this won't compile. Seems easy to use
> GetSufraceWidth and GetSurfaceHeight to avoid.

Actually all I need there is to use nsIntPoint instead of nsPoint...

> >   NS_IMETHOD FrameChanged(imgIContainer *aContainer, gfxIImageFrame *newframe,
> >-                          nsRect * dirtyRect);
> >+                          nsIntRect * dirtyRect);
> 
> Nit: Even though we're all over the place regarding whether its type* x or type
> *x or type * x which is not your fault. We might at least try to be consistent
> within a single function.

:-)

> One other nit which I didn't notice before, you could use some spaces after the
> commas in this assertion.
> 
> NS_ASSERTION(nsIntRect(0,0,aDest->mImage->Width(),aDest->mImage->Height()).Contains(aDataRect),
>                "aDataRect out of bounds");
> 
> Note there are two occurrances of this line in nsSVGFilters.cpp

Fixed.

Pushed 23f7948191ec.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.