54919 2011-02-21 17:37:46 -0800 WebCore should retrieve unclamped frame delays from ImageIO 2011-02-21 18:16:36 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) PC OS X 10.6 RESOLVED FIXED P2 Normal --- 1 mrowe mrowe ap oldest_to_newest 354931 0 mrowe 2011-02-21 17:37:46 -0800 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. 354934 1 83246 mrowe 2011-02-21 17:44:10 -0800 Created attachment 83246 Patch 354939 2 83246 darin 2011-02-21 17:48:18 -0800 Comment on attachment 83246 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. 354940 3 83246 ap 2011-02-21 17:49:55 -0800 Comment on attachment 83246 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)? 354950 4 mrowe 2011-02-21 18:08:07 -0800 (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. 354961 5 mrowe 2011-02-21 18:16:36 -0800 Fixed in r79277. 83246 2011-02-21 17:44:10 -0800 2011-02-21 17:49:55 -0800 Patch 0001-http-webkit.org-b-54919-rdar-problem-7689300-WebCore.patch text/plain 3276 mrowe RnJvbSAxMDgwZTVlZGQ2NzZiMmFhMjU0MzFkYzFmMjIyMzlhNWMyMTM1ZDVhIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBNYXJrIFJvd2UgPG1yb3dlQGFwcGxlLmNvbT4KRGF0ZTogTW9u LCAyMSBGZWIgMjAxMSAxNzo0MToxNyAtMDgwMApTdWJqZWN0OiBbUEFUQ0hdIDxodHRwOi8vd2Vi a2l0Lm9yZy9iLzU0OTE5PiAvIDxyZGFyOi8vcHJvYmxlbS83Njg5MzAwPiBXZWJDb3JlIHNob3Vs ZCByZXRyaWV2ZSB1bmNsYW1wZWQgZnJhbWUgZGVsYXlzIGZyb20gSW1hZ2VJTwoKUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCgoqIHBsYXRmb3JtL2dyYXBoaWNzL2NnL0ltYWdlU291cmNlQ0cu Y3BwOgooV2ViQ29yZTo6SW1hZ2VTb3VyY2U6OmZyYW1lRHVyYXRpb25BdEluZGV4KTogTG9vayBm b3IgdGhlIHVuY2xhbXBlZCBmcmFtZSBkZWxheSBpbiB0aGUKZnJhbWUgcHJvcGVydGllcyBkaWN0 aW9uYXJ5IGFuZCB1c2UgdGhhdCBpZiBpdCBleGlzdHMuIElmIGl0IGRvZXMgbm90IGV4aXN0IGlu IHRoZQpkaWN0aW9uYXJ5IHRoZW4gZmFsbCBiYWNrIHRvIHVzaW5nIHRoZSBjbGFtcGVkIGZyYW1l IGRlbGF5LgotLS0KIFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyAgICAgICAgICAgICAgICAgICAg ICAgICAgIHwgICAxMSArKysrKysrKysrKwogLi4uL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mv Y2cvSW1hZ2VTb3VyY2VDRy5jcHAgfCAgIDEyICsrKysrKysrKystLQogMiBmaWxlcyBjaGFuZ2Vk LCAyMSBpbnNlcnRpb25zKCspLCAyIGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBhNzgzYWIw Li40ZmM3M2U1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTEtMDItMjEgIE1hcmsg Um93ZSAgPG1yb3dlQGFwcGxlLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICA8aHR0cDovL3dlYmtpdC5vcmcvYi81NDkxOT4gLyA8cmRhcjovL3By b2JsZW0vNzY4OTMwMD4gV2ViQ29yZSBzaG91bGQgcmV0cmlldmUgdW5jbGFtcGVkIGZyYW1lIGRl bGF5cyBmcm9tIEltYWdlSU8KKworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL2NnL0ltYWdl U291cmNlQ0cuY3BwOgorICAgICAgICAoV2ViQ29yZTo6SW1hZ2VTb3VyY2U6OmZyYW1lRHVyYXRp b25BdEluZGV4KTogTG9vayBmb3IgdGhlIHVuY2xhbXBlZCBmcmFtZSBkZWxheSBpbiB0aGUKKyAg ICAgICAgZnJhbWUgcHJvcGVydGllcyBkaWN0aW9uYXJ5IGFuZCB1c2UgdGhhdCBpZiBpdCBleGlz dHMuIElmIGl0IGRvZXMgbm90IGV4aXN0IGluIHRoZQorICAgICAgICBkaWN0aW9uYXJ5IHRoZW4g ZmFsbCBiYWNrIHRvIHVzaW5nIHRoZSBjbGFtcGVkIGZyYW1lIGRlbGF5LgorCiAyMDExLTAyLTIx ICBQcmF0aWsgU29sYW5raSAgPHBzb2xhbmtpQGFwcGxlLmNvbT4KIAogICAgICAgICBSZXZpZXdl ZCBieSBEYXJpbiBBZGxlci4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dy YXBoaWNzL2NnL0ltYWdlU291cmNlQ0cuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3Jh cGhpY3MvY2cvSW1hZ2VTb3VyY2VDRy5jcHAKaW5kZXggZmM3Y2QzZS4uNzdkNThhNyAxMDA2NDQK LS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2cvSW1hZ2VTb3VyY2VDRy5j cHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2cvSW1hZ2VTb3VyY2VD Ry5jcHAKQEAgLTQyLDYgKzQyLDEwIEBAIG5hbWVzcGFjZSBXZWJDb3JlIHsKIAogc3RhdGljIGNv bnN0IENGU3RyaW5nUmVmIGtDR0ltYWdlU291cmNlU2hvdWxkUHJlZmVyUkdCMzIgPSBDRlNUUigi a0NHSW1hZ2VTb3VyY2VTaG91bGRQcmVmZXJSR0IzMiIpOwogCisjaWYgZGVmaW5lZChCVUlMRElO R19PTl9USUdFUikgfHwgZGVmaW5lZChCVUlMRElOR19PTl9MRU9QQVJEKSB8fCBkZWZpbmVkKEJV SUxESU5HX09OX1NOT1dfTEVPUEFSRCkKK3N0YXRpYyBjb25zdCBDRlN0cmluZ1JlZiBrQ0dJbWFn ZVByb3BlcnR5R0lGVW5jbGFtcGVkRGVsYXlUaW1lID0gQ0ZTVFIoIlVuY2xhbXBlZERlbGF5VGlt ZSIpOworI2VuZGlmCisKICNpZiAhUExBVEZPUk0oTUFDKQogc2l6ZV90IHNoYXJlZEJ1ZmZlckdl dEJ5dGVzQXRQb3NpdGlvbih2b2lkKiBpbmZvLCB2b2lkKiBidWZmZXIsIG9mZl90IHBvc2l0aW9u LCBzaXplX3QgY291bnQpCiB7CkBAIC0zMTIsOCArMzE2LDEyIEBAIGZsb2F0IEltYWdlU291cmNl OjpmcmFtZUR1cmF0aW9uQXRJbmRleChzaXplX3QgaW5kZXgpCiAgICAgaWYgKHByb3BlcnRpZXMp IHsKICAgICAgICAgQ0ZEaWN0aW9uYXJ5UmVmIHR5cGVQcm9wZXJ0aWVzID0gKENGRGljdGlvbmFy eVJlZilDRkRpY3Rpb25hcnlHZXRWYWx1ZShwcm9wZXJ0aWVzLmdldCgpLCBrQ0dJbWFnZVByb3Bl cnR5R0lGRGljdGlvbmFyeSk7CiAgICAgICAgIGlmICh0eXBlUHJvcGVydGllcykgewotICAgICAg ICAgICAgQ0ZOdW1iZXJSZWYgbnVtID0gKENGTnVtYmVyUmVmKUNGRGljdGlvbmFyeUdldFZhbHVl KHR5cGVQcm9wZXJ0aWVzLCBrQ0dJbWFnZVByb3BlcnR5R0lGRGVsYXlUaW1lKTsKLSAgICAgICAg ICAgIGlmIChudW0pCisgICAgICAgICAgICAvLyBMb29rIGZvciB0aGUgdW5jbGFtcGVkIGZyYW1l IGRlbGF5IGlmIGl0IGV4aXN0cy4KKyAgICAgICAgICAgIGlmIChDRk51bWJlclJlZiBudW0gPSAo Q0ZOdW1iZXJSZWYpQ0ZEaWN0aW9uYXJ5R2V0VmFsdWUodHlwZVByb3BlcnRpZXMsIGtDR0ltYWdl UHJvcGVydHlHSUZVbmNsYW1wZWREZWxheVRpbWUpKQorICAgICAgICAgICAgICAgIENGTnVtYmVy R2V0VmFsdWUobnVtLCBrQ0ZOdW1iZXJGbG9hdFR5cGUsICZkdXJhdGlvbik7CisKKyAgICAgICAg ICAgIC8vIEZhbGwgYmFjayB0byB0aGUgY2xhbXBlZCBmcmFtZSBkZWxheSBpZiB0aGUgdW5jbGFt cGVkIGZyYW1lIGRlbGF5IGRvZXMgbm90IGV4aXN0LgorICAgICAgICAgICAgZWxzZSBpZiAoQ0ZO dW1iZXJSZWYgbnVtID0gKENGTnVtYmVyUmVmKUNGRGljdGlvbmFyeUdldFZhbHVlKHR5cGVQcm9w ZXJ0aWVzLCBrQ0dJbWFnZVByb3BlcnR5R0lGRGVsYXlUaW1lKSkKICAgICAgICAgICAgICAgICBD Rk51bWJlckdldFZhbHVlKG51bSwga0NGTnVtYmVyRmxvYXRUeXBlLCAmZHVyYXRpb24pOwogICAg ICAgICB9CiAgICAgfQotLSAKMS43LjMuNAoK