In bug 26455 we changed WebCore’s animated GIF frame delay clamping to match Firefox. However, ImageIO implements its own frame delay clamping on the value it returns for the kCGImagePropertyGIFDelayTime. We should switch to retrieving the raw, unclamped frame delay from ImageIO so that we really do match Firefox when using ImageIO.
Created attachment 83246 [details] Patch
Comment on attachment 83246 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83246&action=review > Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:319 > + // Look for the unclamped frame delay if it exists. The phrasing here is awkward. We always look for it, but only find it if it exists.
Comment on attachment 83246 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83246&action=review > Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:46 > +static const CFStringRef kCGImagePropertyGIFUnclampedDelayTime = CFSTR("UnclampedDelayTime"); I suggest omitting the unnecessary "static". I see kCGImagePropertyGIFUnclampedDelayTime in some versions of public Snow Leopard headers. Will this break the build for those who have a new version? > Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:324 > + CFNumberGetValue(num, kCFNumberFloatType, &duration); > + > + // Fall back to the clamped frame delay if the unclamped frame delay does not exist. > + else if (CFNumberRef num = (CFNumberRef)CFDictionaryGetValue(typeProperties, kCGImagePropertyGIFDelayTime)) The blank line could confuse someone. Maybe move comments inside the block (and add braces then)?
(In reply to comment #3) > (From update of attachment 83246 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=83246&action=review > > > Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:46 > > +static const CFStringRef kCGImagePropertyGIFUnclampedDelayTime = CFSTR("UnclampedDelayTime"); > > I suggest omitting the unnecessary "static". > > I see kCGImagePropertyGIFUnclampedDelayTime in some versions of public Snow Leopard headers. Will this break the build for those who have a new version? It will. I’ll change this to always provide and use our own version of this constant.
Fixed in r79277.