Closed Bug 625237 Opened 14 years ago Closed 14 years ago

DecrementAnimationConsumers not always called in nsDocument::RemoveImage

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: azakai, Assigned: azakai)

Details

Attachments

(1 file, 1 obsolete file)

Due to a silly bug with a return before it exiting the function.
Attached patch patch (obsolete) — Splinter Review
Asking for approval since without this patch, we may run animations for images after they are no longer shown.
Attachment #503344 - Flags: review?(bzbarsky)
Attachment #503344 - Flags: approval2.0?
Comment on attachment 503344 [details] [diff] [review] patch Should we DecrementAnimationConsumers even if UnlockImage fails? Seems like we should....
If so then we should change the corresponding case in ::AddImage as well. It seems odd though, to do anything after we have a failure in Lock/UnlockImage. Also, what would the final rv be? If just one fails then that one I guess, but what if both fail?
> It seems odd though, to do anything after we have a failure in Lock/UnlockImage. Well, it depends on whether unlock can fail in "sane" situations. Can it? > If just one fails then that one I guess, Right. > but what if both fail? "Pick one". Callers probably don't care what the error code is much. ;)
Attached patch patch v2Splinter Review
Made the requested changes.
Attachment #503344 - Attachment is obsolete: true
Attachment #504795 - Flags: review?(bzbarsky)
Attachment #503344 - Flags: review?(bzbarsky)
Attachment #503344 - Flags: approval2.0?
Comment on attachment 504795 [details] [diff] [review] patch v2 >@@ -8110,32 +8110,34 @@ nsDocument::AddImage(imgIRequest* aImage >+ if ((oldCount == 0) && mAnimatingImages) { Don't add the extra parens there, please. >@@ -8150,27 +8152,31 @@ nsDocument::RemoveImage(imgIRequest* aIm >+ if ((count == 0) && mAnimatingImages) Same here. r=me with those nits fixed.
Attachment #504795 - Flags: review?(bzbarsky) → review+
Requesting blocking-fennec, since without this patch we may run animations for images after they are no longer shown, which is very bad for battery life (I have seen this happen while debugging another imagelib issue).
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: