With the fix of Bug 85595, two functions were added, ZeroMaskArea and OneMaskArea. These functions can be optimized and combined into one. The new function will be called SetMaskVisibility. In addition, ZeroMask will be renamed to SetMaskVisibility as well to keep everything consistant. This will probably happen after the GIF stuff in imgContainer gets moved to imgContainerGIF (Bug 77497)

-Remove ZeroMask, ZeroMaskArea, OneMaskArea -Add SetVisibility counterparts -add some doxygen comments (more to come in other bug patches)

Removed debug code and cut long lines into two.

Comment on attachment 102192 [details] [diff] [review] SetMaskInvisibility The memset optimization is a bit ugly - how often do we actually get a count of one?

please remove the @brief, as discussed on irc nice, didn't know \overload. thanks for making me look it up :) however: from http://www.stack.nl/~dimitri/doxygen/commands.html#cmdoverload : Note 2: The \overload command does not work inside a one-line comment. does that apply here? will look at code issues later.

about the patch itself: PRUint32 abprComposite; aFrame->GetAlphaBytesPerRow(&abprComposite); as you renamed the argument, I think just "abpr" is a better name for this function. same for compositingAlphaData and {width,height}Composite PR_MIN(aWidth,(widthComposite-aX)); please insert a space after the , and around the - same on the next line and here around the *: offset = aY*abprComposite; // Should never get run, since gifs are all 1 bit alpha so why implement it? especially now that this container will only be used for GIFs. I would put an NS_NOTREACHED("gifs must be 1 bit alpha"); in the default: block and remove the RGB_A8 and BGR_A8 block. um, I notice this comment in your patch (though you didn't touch it) // Yet another thing that should be in a GIF specific container. seems out of data, given that this is imgContainerGIF.cpp :) also I agree with tor not quite done with the review, will continue later

PRUint8 maskStart; this variable will only be initialized if width > numReplacingStart and maskShiftStartBy != 0. I think you should initialize it here. similar for maskStart also, both for maskStart and for maskEnd, I would put the ~maskStart/~maskEnd into the if, where it will be used, not in the assignment. i.e., change maskStart = (aVisible) ? (0xFF >> maskShiftStartBy) : ~(0xFF >> maskShiftStartBy); to maskStart = (0xFF >> maskShiftStartBy); (same for the other assignments to maskStart) and later change: *localAlpha++ &= maskStart; to *localAlpha++ &= ~maskStart; (same for maskEnd) that makes it imho more clear what happens there.

note, the ResetAnimation patch has bitrotted this one - that function needs a change as well (trivial to do)

tor: my pretty 1 byte setting optimization is gone :P biesi: re: overload not working on single line. That's very odd, because it's working for me. I think they lie. :P Either that or it works in this case because it's right above the function. The example they gave their code documentation was seperate from the class code. re: abrComposite oh, blast me and my changing of the parameter from aCompositingFrame to aFrame without updating the other variable names. ;) Everything with Composit* has been removed now. re: initializing maskStart maskStart is only used if maskShiftStartBy != 0, and maskStart is initialized for all those cases. not sure what you are refering to when you say "similar for maskStart" re: // Yet another thing.. Yes, those will be removed in the other imgContainerGIF patches I have Other Changes: - Moved abpr into case block - Set alphaLine in WIN&OS2/Other #define so that we always increase alphaLine, thus not needing more #define switches in the loops (making it easier to read) - Moved maskShiftEndBy declaration into the code block it's used in - Split the for loop into two, one for aVisible and one for !aVisible. Before aVisible checked every loop incrementation. - ((Removed (excess ((bracketting)))!)) - unbitrotted

> similar for maskStart er, right, I meant something else, presumably maskEnd ok, if the @overload works for you, that's fine.

Comment on attachment 102714 [details] [diff] [review] SetMaskVisibility r=biesi

Comment on attachment 102714 [details] [diff] [review] SetMaskVisibility er, forgot to mention, please remove the unused offset variable as mentioned on irc

Comment on attachment 102714 [details] [diff] [review] SetMaskVisibility Remove offset per biesi's comment. Add an initializer to maskStart to quiet the compiler. sr=tor

removed offset and initialized maskStart (although my compiler doesn't give a warning)

Comment on attachment 102750 [details] [diff] [review] SetMaskVisibility Transferring r=biesi, sr=tor from previous patch

Checked in.

Closing.