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

RESOLVED FIXED in mozilla21

Status

()

Core
ImageLib
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

Trunk
mozilla21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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);
(Assignee)

Updated

5 years ago
Summary: Replace imgIContainer::GetCurrentFrameIsOpaque() with [noscript] GetFrameIsOpaque(aWhichFrame) → Replace imgIContainer::GetCurrentFrameIsOpaque() with [noscript] FrameIsOpaque(aWhichFrame)
(Assignee)

Comment 1

5 years ago
Created attachment 707917 [details] [diff] [review]
Replace GetCurrentFrameIsOpaque() with [noscript] FrameIsOpaque(aWhichFrame).

Proposed patch. Try job here: https://tbpl.mozilla.org/?tree=Try&rev=1dcc12493d9d
Attachment #707917 - Flags: review?(joe)
(Assignee)

Comment 2

5 years ago
Created attachment 707923 [details] [diff] [review]
Replace GetCurrentFrameIsOpaque() with [noscript] FrameIsOpaque(aWhichFrame).

Whoops! Forgot to change the imgIContainer UUID. No need for a new try run.
Attachment #707923 - Flags: review?(joe)
(Assignee)

Updated

5 years ago
Attachment #707917 - Attachment is obsolete: true
Attachment #707917 - Flags: review?(joe)
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.
Attachment #707923 - Flags: review?(joe) → review+
(Assignee)

Comment 4

5 years ago
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!
(Assignee)

Comment 5

5 years ago
Created attachment 709899 [details] [diff] [review]
Replace GetCurrentFrameIsOpaque() with [noscript] FrameIsOpaque(aWhichFrame).

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

Updated

5 years ago
Attachment #707923 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
This should be good to go. Requesting checkin.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/04be5f5f4b18
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.