803349 - Restore NS_DECL_IMGICONTAINER to VectorImage/RasterImage

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Restore NS_DECL_IMGICONTAINER to VectorImage / RasterImage.

Proposed patch.

Comment 2

[Seth Fowler [:seth] [:s2h]]

There's a try run a-trying at https://tbpl.mozilla.org/?tree=Try&rev=b00d03636b04.

Comment 3

[Joe Drew (not getting mail)]

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.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: Restore NS_DECL_IMGICONTAINER to VectorImage / RasterImage.

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.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Ah, forgot to mention in the previous comment: carrying over r+. This should be ready to land; the try run looks OK. Requesting checkin.

Comment 6

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b47dfc639029

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b47dfc639029