Closed Bug 185773 Opened 22 years ago Closed 22 years ago

Call SetTimeout in nsGIFDecoder2, not imgContainerGIF::EndFrameDecode


(Core :: Graphics: ImageLib, defect)

Not set





(Reporter: paper, Assigned: paper)




(1 file)

imgContainerGIF::EndFrameDecode recieves aFrameNum and aTimeout as parameters. 
It then retrieves the currentFrame object using aFrameNum, and sets the timeout
to aTimeout.

This is plain silly.  nsGIFDecoder2 has a reference to the frame, plus it has
the timeout value.  nsGIFDecoder2 can set the timeout, and save imgContainerGIF
from wasting a CPU cycle (or two!) retrieving the frame object from the frame
Move SetTimeout call from imgContainerGIF (chunk 3 of file 2) to nsGIFDecoder2
(chunk 1 of file 1).  The rest of the chunks are formatting and compiler
warning removals that I thought I'd throw in just to confuse reviewers. :P
Comment on attachment 109515 [details] [diff] [review]
Move SetTimeout call

biesi! You know you love GIF code reviewing ;)

I promise it won't be as nasty as BMP reviewing :)
Attachment #109515 - Flags: review?(cbiesinger)
Comment on attachment 109515 [details] [diff] [review]
Move SetTimeout call

+    if (!mTimer) {

mTimer will always be null here, because if it isn't, you are returning early
from the function. what are you adding this check for?

-  // don't bother trying to change the frame (to 0, etc.) here.
-  // No one is listening.
why are you removing this comment?

+  PRInt32 numFrames = inlinedGetNumFrames();
well in my tree this is an unsigned integer... why are you assigning it to a
signed one?
should inlinedGetNumFrames maybe return a PRInt32?

the rest looks good.
Attachment #109515 - Flags: review?(cbiesinger) → review-
You're right, no need for that (!mTimer) check.  I've removed it.

The removed comments are misleading.  You could in theory reset to frame 0 at
that point.. there's always a change StartAnimation will be called again.  (You
wouldn't want to set it to frame 0, but that's not the point)

+  PRInt32 numFrames = inlinedGetNumFrames();
This is a temporary fix to shut up the unsigned/signed compiler warning.  I can
removed this change if you wish, as that whole function will be getting a
rewrite soon.  I think there is no use "fixing it properly" if the function is
going to be rewritten.
Comment on attachment 109515 [details] [diff] [review]
Move SetTimeout call

ok, if that will be rewritten anyway...

no need to attach a new patch just for that other change, then.
Attachment #109515 - Flags: review- → review+
Comment on attachment 109515 [details] [diff] [review]
Move SetTimeout call

sr=tor with biesi's comments, which I would have liked to see as a new

In the future, please keep the number of unrelated indentation changes
and code tweaks to a minimum, as they just slow down the process of
doing reviews.
Attachment #109515 - Flags: superreview+
checked-in 2002-12-29
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.