Last Comment Bug 691754 - Change the decoder constructor to use a c++ reference instead of a pointer
: Change the decoder constructor to use a c++ reference instead of a pointer
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Jeff Muizelaar [:jrmuizel]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-04 07:49 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2011-10-21 08:35 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Change the decoder constructor to use a c++ reference instead of a pointer (31.19 KB, patch)
2011-10-04 07:49 PDT, Jeff Muizelaar [:jrmuizel]
joe: review+
Details | Diff | Review

Description Jeff Muizelaar [:jrmuizel] 2011-10-04 07:49:28 PDT
Created attachment 564548 [details] [diff] [review]
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] (largely AFK until June 28) 2011-10-04 14:31:42 PDT
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] (largely AFK until June 28) 2011-10-04 14:32:16 PDT
(In reply to Daniel Holbert [:dholbert] from comment #1)
> 2 drive-by nits:

(er "3")
Comment 3 Joe Drew (not getting mail) 2011-10-04 14:38:34 PDT
(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) 2011-10-05 10:03:53 PDT
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] 2011-10-21 08:35:02 PDT
https://hg.mozilla.org/mozilla-central/rev/7eefb2f21c4c

Note You need to log in before you can comment on or make changes to this bug.