The default bug view has changed. See this FAQ.

RasterImage::Anim::ensureAnimExists() should be cleaned up

RESOLVED FIXED in mozilla8

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jwir3, Assigned: jwir3)

Tracking

Trunk
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment)

3.36 KB, patch
dholbert
: review+
Joe Drew (not getting mail)
: review+
Justin Lebar (not reading bugmail)
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
RasterImage::Anim::ensureAnimExists() is a function used for validation of animation objects. It should be capitalized and modified to return void, as described in bug 666446 comment 75:

> RasterImage::RequestRefresh(const mozilla::TimeStamp& aTime)
> {
>+  this->ensureAnimExists();
>+  if (!mAnimating || !ShouldAnimate()) {
>+    return;
>+  }
 
It looks like ensureAnimExists thinks it can fail (that is, we have failure-checks for it in other places) -- however, infallible-new means it really can't fail.  To avoid confusion about where/whether we need to failure-check it, perhaps we should just make ensureAnimExists() return void? (and capitalize it to follow convention? I think it's the only non-capitalized method in RasterImage.)
(Assignee)

Updated

6 years ago
Assignee: nobody → sjohnson
(Assignee)

Updated

6 years ago
Blocks: 666446
Component: Layout: Images → ImageLib
QA Contact: layout.images → imagelib
(Assignee)

Comment 1

6 years ago
Created attachment 547960 [details] [diff] [review]
Patch 0 - Refactoring that fixes this bug

Refactoring that fixes this bug.
Attachment #547960 - Flags: review?(dholbert)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Comment on attachment 547960 [details] [diff] [review]
Patch 0 - Refactoring that fixes this bug

Looks fine to me!

joe should sign off on this -- flagging him for addl. review.
Attachment #547960 - Flags: review?(joe)
Attachment #547960 - Flags: review?(dholbert)
Attachment #547960 - Flags: review+
Comment on attachment 547960 [details] [diff] [review]
Patch 0 - Refactoring that fixes this bug

Review of attachment 547960 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #547960 - Flags: review?(joe) → review+
(Assignee)

Comment 4

6 years ago
Comment on attachment 547960 [details] [diff] [review]
Patch 0 - Refactoring that fixes this bug

Requesting this be landed.

Here's a link to the try server results: 
http://tbpl.mozilla.org/?tree=Try&rev=630e13ab9ea4

Please note that the tests that failed were intermittent failures, and were re-run with all but 1 successful. The build that failed was apparently also an intermittent failure, as I re-ran it and it built fine.
Attachment #547960 - Flags: checkin?(joe)
Inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/54d6d13e664b
Whiteboard: [inbound]
Attachment #547960 - Flags: checkin?(joe) → checkin+
http://hg.mozilla.org/mozilla-central/rev/54d6d13e664b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.