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

RESOLVED FIXED in mozilla10

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

Trunk
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
Attachment #564548 - Flags: review?(joe)
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)
(In reply to Daniel Holbert [:dholbert] from comment #1)
> 2 drive-by nits:

(er "3")
(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 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
Attachment #564548 - Flags: review?(joe) → review+
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/7eefb2f21c4c
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.