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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

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 1

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

Attachment: Replace GetCurrentFrameRect() with FrameRect(aWhichFrame).

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

Comment 2

[Joe Drew (not getting mail)]

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.

Comment 3

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

You got it, Joe. I'll make the change. Thanks for the review.

Comment 4

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

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.

Comment 5

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

Attachment: Replace GetCurrentFrameRect() with FrameRect(aWhichFrame).

Made review changes. Shouldn't require another try run.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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