Last Comment Bug 673535 - RasterImage::Anim::ensureAnimExists() should be cleaned up
: RasterImage::Anim::ensureAnimExists() should be cleaned up
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Scott Johnson (:jwir3)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: 666446
  Show dependency treegraph
 
Reported: 2011-07-22 13:46 PDT by Scott Johnson (:jwir3)
Modified: 2011-08-03 02:11 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 0 - Refactoring that fixes this bug (3.36 KB, patch)
2011-07-23 14:49 PDT, Scott Johnson (:jwir3)
dholbert: review+
joe: review+
justin.lebar+bug: checkin+
Details | Diff | Splinter Review

Description Scott Johnson (:jwir3) 2011-07-22 13:46:18 PDT
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.)
Comment 1 Scott Johnson (:jwir3) 2011-07-23 14:49:55 PDT
Created attachment 547960 [details] [diff] [review]
Patch 0 - Refactoring that fixes this bug

Refactoring that fixes this bug.
Comment 2 Daniel Holbert [:dholbert] 2011-07-23 15:17:05 PDT
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.
Comment 3 Joe Drew (not getting mail) 2011-07-28 11:20:38 PDT
Comment on attachment 547960 [details] [diff] [review]
Patch 0 - Refactoring that fixes this bug

Review of attachment 547960 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 4 Scott Johnson (:jwir3) 2011-08-02 08:36:04 PDT
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.
Comment 5 Justin Lebar (not reading bugmail) 2011-08-02 09:26:30 PDT
Inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/54d6d13e664b
Comment 6 Marco Bonardo [::mak] 2011-08-03 02:11:55 PDT
http://hg.mozilla.org/mozilla-central/rev/54d6d13e664b

Note You need to log in before you can comment on or make changes to this bug.