Closed
Bug 625237
Opened 14 years ago
Closed 14 years ago
DecrementAnimationConsumers not always called in nsDocument::RemoveImage
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
fennec | 2.0+ | --- |
People
(Reporter: azakai, Assigned: azakai)
Details
Attachments
(1 file, 1 obsolete file)
2.46 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Due to a silly bug with a return before it exiting the function.
Assignee | ||
Comment 1•14 years ago
|
||
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 2•14 years ago
|
||
Comment on attachment 503344 [details] [diff] [review]
patch
Should we DecrementAnimationConsumers even if UnlockImage fails? Seems like we should....
Assignee | ||
Comment 3•14 years ago
|
||
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?
![]() |
||
Comment 4•14 years ago
|
||
> 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. ;)
Assignee | ||
Comment 5•14 years ago
|
||
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 6•14 years ago
|
||
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+
Assignee | ||
Comment 7•14 years ago
|
||
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: --- → ?
Updated•14 years ago
|
tracking-fennec: ? → 2.0+
Assignee | ||
Comment 8•14 years ago
|
||
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.
Description
•