691754 - Change the decoder constructor to use a c++ reference instead of a pointer

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Change the decoder constructor to use a c++ reference instead of a pointer

A raster image always outlives it's decoder so we can refer to it using a reference instead of a pointer.

Comment 1

[Daniel Holbert [:dholbert]]

Comment on attachment 564548 [details] [diff] [review]
Change the decoder constructor to use a c++ reference instead of a pointer

2 drive-by nits:

>-            rv = mImage->EnsureFrame(0, 0, 0, mBIH.width, real_height, 
>+            rv = mImage.EnsureFrame(0, 0, 0, mBIH.width, real_height, 
>                                      gfxASurface::ImageFormatARGB32,
>                                      (PRUint8**)&mImageData, &imageLength);

Looks like these s/->/./ changes would like some spacing tweaks on the subsequent lines.
(maybe you already know that and are just attaching a diff -w version to be nice though)

>+Decoder::Decoder(RasterImage &aImage, imgIDecoderObserver* aObserver)
>+  : mImage(aImage)
>+  , mDecodeFlags(0)
>   , mFrameCount(0)
>   , mFailCode(NS_OK)
>   , mInitialized(false)
>   , mSizeDecode(false)
>   , mInFrame(false)
>   , mDecodeDone(false)
>   , mDataError(false)
> {
>-  // We should always have an image
>-  NS_ABORT_IF_FALSE(aImage, "Can't initialize decoder without an image!");
>-
>   // Save our paremeters
>-  mImage = aImage;
>   mObserver = aObserver;
> }

Can't mObserver be moved up into the init list, too, leaving the constructor nice and empty?

>+++ b/modules/libpr0n/src/Decoder.h
> class Decoder
> {
> public:
> 
>-  Decoder(RasterImage* aImage, imgIDecoderObserver* aObserver);
>+  Decoder(RasterImage& aImage, imgIDecoderObserver* aObserver);

Extreme-nit: the '&' character is type-hugging in the method decl here, but it's variable-name-hugging in all the method impls.  ("RasterImage&" vs. "RasterImage &") This is an already-existing (super-minor) problem ("RasterImage*" vs "RasterImage *"), but it might be nice to clean that up as long as you're touching all these method-signatures)

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> 2 drive-by nits:

(er "3")

Comment 3

[Joe Drew (not getting mail)]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> Extreme-nit: the '&' character is type-hugging in the method decl here, but
> it's variable-name-hugging in all the method impls.  ("RasterImage&" vs.
> "RasterImage &") This is an already-existing (super-minor) problem
> ("RasterImage*" vs "RasterImage *"), but it might be nice to clean that up
> as long as you're touching all these method-signatures)

Type-hugging is what I prefer FWIW.

Comment 4

[Joe Drew (not getting mail)]

Comment on attachment 564548 [details] [diff] [review]
Change the decoder constructor to use a c++ reference instead of a pointer

Review of attachment 564548 [details] [diff] [review]:
-----------------------------------------------------------------

Make formatting changes as dholbert and I said and r=me

Comment 5

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/7eefb2f21c4c