Closed Bug 836155 Opened 7 years ago Closed 7 years ago

Image::GetCurrentFrameRect should become Image::FrameRect(aWhichFrame)

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: seth, Assigned: seth)

Details

Attachments

(1 file, 1 obsolete file)

For some instances of an image to ignore animation, we need to eliminate as much as possible imgIContainer/Image methods that always act on the current frame. |GetCurrentFrameRect| is one such method. We should give it an |aWhichFrame| argument and rename it to |FrameRect|.
Comment on attachment 707957 [details] [diff] [review]
Replace GetCurrentFrameRect() with FrameRect(aWhichFrame).

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

I feel relatively strongly about the below comment, but I don't need to see this afterwards, so r=me.

::: image/src/Image.h
@@ +69,2 @@
>     */
> +  virtual nsresult FrameRect(uint32_t aWhichFrame, nsIntRect& aRect) = 0;

Oh, I am not happy about having an nsresult return value. I'd much rather return an empty rect in the error case.
Attachment #707957 - Flags: review?(joe) → review+
You got it, Joe. I'll make the change. Thanks for the review.
Incidentally, the one thing we lose here is the ability to tell the caller that they passed in an invalid FRAME_* enumeration value, but as I mentioned in bug 836124, that doesn't seem important, since any code that does that is just wrong. I'll make that print an NS_WARNING instead.
Made review changes. Shouldn't require another try run.
Attachment #707957 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e9b342a7e9b8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.