836124 - Replace imgIContainer::GetCurrentFrameIsOpaque() with [noscript] FrameIsOpaque(aWhichFrame)

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

The |currentFrameIsOpaque| attribute on imgIContainer is not used by any addons in the addons MXR, nor by any JavaScript code of ours. It should be marked |noscript|. Furthermore, it is preferable that all methods which operate on the current frame should be replaced with versions which take an |aWhichFrame| argument, as this makes it much easier to ignore animation on particular instances of images. So the overall proposed change here is to go from this:

> readonly attribute boolean currentFrameIsOpaque;

To this:

> [noscript] boolean frameIsOpaque(in uint32_t aWhichFrame);

Comment 1

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

Attachment: Replace GetCurrentFrameIsOpaque() with [noscript] FrameIsOpaque(aWhichFrame).

Proposed patch. Try job here: https://tbpl.mozilla.org/?tree=Try&rev=1dcc12493d9d

Comment 2

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

Attachment: Replace GetCurrentFrameIsOpaque() with [noscript] FrameIsOpaque(aWhichFrame).

Whoops! Forgot to change the imgIContainer UUID. No need for a new try run.

Comment 3

[Joe Drew (not getting mail)]

Comment on attachment 707923 [details] [diff] [review]
Replace GetCurrentFrameIsOpaque() with [noscript] FrameIsOpaque(aWhichFrame).

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

::: image/public/imgIContainer.idl
@@ +151,5 @@
> +   * behind it.
> +   *
> +   * @param aWhichFrame Frame specifier of the FRAME_* variety.
> +   */
> +  [noscript] boolean frameIsOpaque(in uint32_t aWhichFrame);

If you make this notxpcom too, it'll have a boolean return value instead of nsresult. Up to you.

Comment 4

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

I'm fine with that. The only thing we lose is the ability to signal the caller that they passed an invalid FRAME_* enumeration value in, which doesn't seem that important because any code that does that is just wrong. I'll make that print an NS_WARNING instead. Thanks for the review!

Comment 5

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

Attachment: Replace GetCurrentFrameIsOpaque() with [noscript] FrameIsOpaque(aWhichFrame).

Switched from [noscript] to [notxpcom]. Should be OK without another try run.

Comment 6

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

This should be good to go. Requesting checkin.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04be5f5f4b18

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/04be5f5f4b18