185773 - Call SetTimeout in nsGIFDecoder2, not imgContainerGIF::EndFrameDecode

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[ArronM (:paper)]

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
number.

Comment 1

[ArronM (:paper)]

Attachment: Move SetTimeout call

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 2

[ArronM (:paper)]

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 :)

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 4

[ArronM (:paper)]

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 5

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 6

[tor]

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
attachment.

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.

Comment 7

[ArronM (:paper)]

checked-in 2002-12-29