Closed Bug 803349 Opened 7 years ago Closed 7 years ago

Restore NS_DECL_IMGICONTAINER to VectorImage/RasterImage

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file, 1 obsolete file)

In the process of working on bug 790640 I noticed that RasterImage.h and VectorImage.h have incomplete copies of the expansion of NS_DECL_IMGICONTAINER which have to be manually kept in sync with the IDL file. This was introduced by bug 611797, which moved GetAnimationMode and SetAnimationMode to Image so that RasterImage and VectorImage can share the implementation.

To make all our lives easier I propose the alternate solution that we return to using NS_DECL_IMGICONTAINER in these files and simply provide an inline implementation that calls the shared code implemented on the Image superclass. This should make maintenance easier with no real downsides.
Proposed patch.
Attachment #673008 - Flags: review?(joe)
There's a try run a-trying at https://tbpl.mozilla.org/?tree=Try&rev=b00d03636b04.
Comment on attachment 673008 [details] [diff] [review]
Restore NS_DECL_IMGICONTAINER to VectorImage / RasterImage.

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

Some small changes needed, but overall nice. Thanks!

::: image/src/Image.cpp
@@ +101,5 @@
>    EvaluateAnimation();
>  }
>  
>  //******************************************************************************
>  /* attribute unsigned short animationMode; */

Remove this IDL stuff since it no longer applies

@@ +102,5 @@
>  }
>  
>  //******************************************************************************
>  /* attribute unsigned short animationMode; */
>  NS_IMETHODIMP

When you remove the NS_IMETHOD in the .h file you'll need to change this to bare nsresult

@@ +115,5 @@
>    return NS_OK;
>  }
>  
>  //******************************************************************************
>  /* attribute unsigned short animationMode; */

Remove this IDL stuff since it no longer applies

@@ +116,5 @@
>  }
>  
>  //******************************************************************************
>  /* attribute unsigned short animationMode; */
>  NS_IMETHODIMP

When you remove the NS_IMETHOD in the .h file you'll need to change this to bare nsresult

::: image/src/Image.h
@@ +97,5 @@
>  
>  protected:
>    Image(imgStatusTracker* aStatusTracker);
>  
> +  // Shared functionality from NS_DECL_IMGICONTAINER:

Reword this comment so that it's more accurate, something along the lines of "Shared functionality for implementers of imgIContainer to forward to when implementing attribute animationMode"

@@ +99,5 @@
>    Image(imgStatusTracker* aStatusTracker);
>  
> +  // Shared functionality from NS_DECL_IMGICONTAINER:
> +  NS_IMETHOD GetAnimationModeInternal(uint16_t *aAnimationMode);
> +  NS_IMETHOD SetAnimationModeInternal(uint16_t aAnimationMode);

They're not interface method so they should just return bare nsresult.

::: image/src/VectorImage.cpp
@@ +386,5 @@
> +NS_IMETHODIMP
> +VectorImage::GetImageContainer(mozilla::layers::ImageContainer** _retval)
> +{
> +  *_retval = nullptr;
> +  return NS_OK;

This part seems unrelated - was it unimplemented before?! scary.
Attachment #673008 - Flags: review?(joe) → review+
Thanks Joe! I've made the changes you suggested.

Regarding VectorImage::GetImageContainer, this was actually implemented inline inside the block of declarations of methods from NS_DECL_IMGICONTAINER in VectorImage.h. Since we switched back to using the macro it couldn't live there anymore, so I moved it to its new, more spacious home.
Attachment #673008 - Attachment is obsolete: true
Attachment #673329 - Flags: review+
Ah, forgot to mention in the previous comment: carrying over r+. This should be ready to land; the try run looks OK. Requesting checkin.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b47dfc639029
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.