68054 2011-09-13 21:08:20 -0700 Return an image scale factor as well as an Image* from CachedImage::brokenImage() 2011-09-14 11:35:10 -0700 1 1 1 Unclassified WebKit Images 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 bdakin bdakin bdakin oldest_to_newest 466813 0 bdakin 2011-09-13 21:08:20 -0700 As discussed in https://bugs.webkit.org/show_bug.cgi?id=67885 , CachedImage::brokenImage() should return the broken image's resolution scale factor as well as the Image itself. This will allow callers to stop hard-coding the image's scale factor. 466819 1 107281 bdakin 2011-09-13 21:20:44 -0700 Created attachment 107281 Patch 466839 2 107281 darin 2011-09-13 22:13:45 -0700 Comment on attachment 107281 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107281&action=review OK but could be improved a bit. > Source/WebCore/loader/cache/CachedImage.cpp:116 > +std::pair<Image*, float> CachedImage::brokenImage(float deviceScaleFactor) const When choosing between returning a pair and using an out argument for the second result, normally we have chosen to use an out argument instead. I think I’m OK with the pair here, though. This should just be pair, not std::pair here inside the cpp file. > Source/WebCore/loader/cache/CachedImage.cpp:120 > + return make_pair(brokenImageHiRes, 2.0f); You should be able to use just "2" rather than "2.0f". It will make a pair of the wrong type, but then it will be converted to the right type. > Source/WebCore/loader/cache/CachedImage.cpp:124 > + return make_pair(brokenImageLoRes, 1.0f); You should be able to use just "1" rather than "1.0f". > Source/WebCore/rendering/RenderImage.cpp:93 > + if (deviceScaleFactor >= 2) > + imageSize.scale(1 / brokenImageAndImageScaleFactor.second); There is no need to put this code inside an if. We can always scale. > Source/WebCore/rendering/RenderImage.cpp:287 > if (deviceScaleFactor >= 2) > - imageSize.scale(0.5f); > + imageSize.scale(1 / brokenImageAndImageScaleFactor.second); There is no need to put this code inside an if. 467173 3 bdakin 2011-09-14 11:32:26 -0700 (In reply to comment #2) > (From update of attachment 107281 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107281&action=review > > OK but could be improved a bit. > > > Source/WebCore/loader/cache/CachedImage.cpp:116 > > +std::pair<Image*, float> CachedImage::brokenImage(float deviceScaleFactor) const > > When choosing between returning a pair and using an out argument for the second result, normally we have chosen to use an out argument instead. I think I’m OK with the pair here, though. > I chose a pair since I have never liked out arguments. I will keep it that way since you're okay with it in this instance. > This should just be pair, not std::pair here inside the cpp file. > Fixed. > > Source/WebCore/loader/cache/CachedImage.cpp:120 > > + return make_pair(brokenImageHiRes, 2.0f); > > You should be able to use just "2" rather than "2.0f". It will make a pair of the wrong type, but then it will be converted to the right type. > Fixed. > > Source/WebCore/loader/cache/CachedImage.cpp:124 > > + return make_pair(brokenImageLoRes, 1.0f); > > You should be able to use just "1" rather than "1.0f". > Fixed. > > Source/WebCore/rendering/RenderImage.cpp:93 > > + if (deviceScaleFactor >= 2) > > + imageSize.scale(1 / brokenImageAndImageScaleFactor.second); > > There is no need to put this code inside an if. We can always scale. > Good point! Fixed. > > Source/WebCore/rendering/RenderImage.cpp:287 > > if (deviceScaleFactor >= 2) > > - imageSize.scale(0.5f); > > + imageSize.scale(1 / brokenImageAndImageScaleFactor.second); > > There is no need to put this code inside an if. Also fixed. Thanks for the review! 467176 4 bdakin 2011-09-14 11:35:10 -0700 Committed change with revision 95103. 107281 2011-09-13 21:20:44 -0700 2011-09-13 22:13:45 -0700 Patch for-review.txt text/plain 6099 bdakin SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDk1MDczKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjQgQEAKKzIwMTEtMDktMTMgIEJldGggRGFr aW4gIDxiZGFraW5AYXBwbGUuY29tPgorCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD02ODA1NAorICAgICAgICBSZXR1cm4gYW4gaW1hZ2Ugc2NhbGUgZmFj dG9yIGFzIHdlbGwgYXMgYW4gSW1hZ2UqIGZyb20gQ2FjaGVkSW1hZ2U6OmJyb2tlbkltYWdlKCkK KworICAgICAgICBObyBuZXcgdGVzdHMuIChPT1BTISkKKworICAgICAgICBDYWNoZWRJbWFnZTo6 YnJva2VuSW1hZ2UoKSBub3cgcmV0dXJucyBhIHBhaXI8SW1hZ2UqLCBmbG9hdD4gd2hlcmUgdGhl IGZsb2F0IAorICAgICAgICByZXByZXNlbnRzIHRoZSBpbWFnZSdzIHNjYWxlIGZhY3Rvci4gVGhp cyBpcyBpbXBvcnRhbnQgYmVjYXVzZSBjdXJyZW50bHksIHRoZSAKKyAgICAgICAgYnJva2VuIGlt YWdlIHdpbGwgZWl0aGVyIGJlIG9ubHkgMXggb3IgMngsIGJ1dCBhIGRldmljZVNjYWxlRmFjdG9y IGNvdWxkIAorICAgICAgICB0aGVvcmV0aWNhbGx5IGJlIHNvbWV0aGluZyBkaWZmZXJlbnQgKDEu NSwgMywgZXRjKS4gU28gaXQgaXMgbm90IHNhZmUgdG8gYXNzdW1lIAorICAgICAgICB0aGF0IHRo ZSBpbWFnZSdzIHNjYWxlIGZhY3RvciBpcyBlcXVpdmFsZW50IHRvIHRoZSBkZXZpY2VTY2FsZUZh Y3RvciwgYW5kIAorICAgICAgICBoYXJkY29kaW5nIDIgZm9yIG5vdyBpcyBsYW1lLgorICAgICAg ICAqIGxvYWRlci9jYWNoZS9DYWNoZWRJbWFnZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpDYWNo ZWRJbWFnZTo6YnJva2VuSW1hZ2UpOgorICAgICAgICAoV2ViQ29yZTo6Q2FjaGVkSW1hZ2U6Omlt YWdlKToKKyAgICAgICAgKiBsb2FkZXIvY2FjaGUvQ2FjaGVkSW1hZ2UuaDoKKyAgICAgICAgKiBy ZW5kZXJpbmcvUmVuZGVySW1hZ2UuY3BwOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVySW1hZ2U6 OmltYWdlU2l6ZUZvckVycm9yKToKKyAgICAgICAgKFdlYkNvcmU6OlJlbmRlckltYWdlOjpwYWlu dFJlcGxhY2VkKToKKwogMjAxMS0wOS0xMyAgQWRhbSBLbGVpbiAgPGFkYW1rQGNocm9taXVtLm9y Zz4KIAogICAgICAgICBGaXggY3NzVGV4dCBwcm9wZXJ0eSBvZiBjb3VudGVyLXZhbHVlZCBDU1NQ cmltaXRpdmVWYWx1ZSBhbmQgYXZvaWQgdW5pbml0aWFsaXplZCByZWFkCkluZGV4OiBTb3VyY2Uv V2ViQ29yZS9sb2FkZXIvY2FjaGUvQ2FjaGVkSW1hZ2UuY3BwCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJj ZS9XZWJDb3JlL2xvYWRlci9jYWNoZS9DYWNoZWRJbWFnZS5jcHAJKHJldmlzaW9uIDk0OTgwKQor KysgU291cmNlL1dlYkNvcmUvbG9hZGVyL2NhY2hlL0NhY2hlZEltYWdlLmNwcAkod29ya2luZyBj b3B5KQpAQCAtMTEzLDE1ICsxMTMsMTUgQEAgdm9pZCBDYWNoZWRJbWFnZTo6YWxsQ2xpZW50c1Jl bW92ZWQoKQogICAgICAgICBtX2RlY29kZWREYXRhRGVsZXRpb25UaW1lci5zdGFydE9uZVNob3Qo aW50ZXJ2YWwpOwogfQogCi1JbWFnZSogQ2FjaGVkSW1hZ2U6OmJyb2tlbkltYWdlKGZsb2F0IGRl dmljZVNjYWxlRmFjdG9yKSBjb25zdAorc3RkOjpwYWlyPEltYWdlKiwgZmxvYXQ+IENhY2hlZElt YWdlOjpicm9rZW5JbWFnZShmbG9hdCBkZXZpY2VTY2FsZUZhY3RvcikgY29uc3QKIHsKICAgICBp ZiAoZGV2aWNlU2NhbGVGYWN0b3IgPj0gMikgewogICAgICAgICBERUZJTkVfU1RBVElDX0xPQ0FM KEltYWdlKiwgYnJva2VuSW1hZ2VIaVJlcywgKEltYWdlOjpsb2FkUGxhdGZvcm1SZXNvdXJjZSgi bWlzc2luZ0ltYWdlQDJ4IikubGVha1JlZigpKSk7Ci0gICAgICAgIHJldHVybiBicm9rZW5JbWFn ZUhpUmVzOworICAgICAgICByZXR1cm4gbWFrZV9wYWlyKGJyb2tlbkltYWdlSGlSZXMsIDIuMGYp OwogICAgIH0KIAogICAgIERFRklORV9TVEFUSUNfTE9DQUwoSW1hZ2UqLCBicm9rZW5JbWFnZUxv UmVzLCAoSW1hZ2U6OmxvYWRQbGF0Zm9ybVJlc291cmNlKCJtaXNzaW5nSW1hZ2UiKS5sZWFrUmVm KCkpKTsKLSAgICByZXR1cm4gYnJva2VuSW1hZ2VMb1JlczsKKyAgICByZXR1cm4gbWFrZV9wYWly KGJyb2tlbkltYWdlTG9SZXMsIDEuMGYpOwogfQogCiBib29sIENhY2hlZEltYWdlOjp3aWxsUGFp bnRCcm9rZW5JbWFnZSgpIGNvbnN0CkBAIC0xMzcsNyArMTM3LDcgQEAgSW1hZ2UqIENhY2hlZElt YWdlOjppbWFnZSgpIGNvbnN0CiAgICAgICAgIC8vIFJldHVybmluZyB0aGUgMXggYnJva2VuIGlt YWdlIGlzIG5vbi1pZGVhbCwgYnV0IHdlIGNhbm5vdCByZWxpYWJseSBhY2Nlc3MgdGhlIGFwcHJv cHJpYXRlCiAgICAgICAgIC8vIGRldmljZVNjYWxlRmFjdG9yIGZyb20gaGVyZS4gSXQgaXMgY3Jp dGljYWwgdGhhdCBjYWxsZXJzIHVzZSBDYWNoZWRJbWFnZTo6YnJva2VuSW1hZ2UoKSAKICAgICAg ICAgLy8gd2hlbiB0aGV5IG5lZWQgdGhlIHJlYWwsIGRldmljZVNjYWxlRmFjdG9yLWFwcHJvcHJp YXRlIGJyb2tlbiBpbWFnZSBpY29uLiAKLSAgICAgICAgcmV0dXJuIGJyb2tlbkltYWdlKDEpOwor ICAgICAgICByZXR1cm4gYnJva2VuSW1hZ2UoMSkuZmlyc3Q7CiAgICAgfQogCiAgICAgaWYgKG1f aW1hZ2UpCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9sb2FkZXIvY2FjaGUvQ2FjaGVkSW1hZ2UuaAo9 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9sb2FkZXIvY2FjaGUvQ2FjaGVkSW1hZ2UuaAko cmV2aXNpb24gOTQ5ODApCisrKyBTb3VyY2UvV2ViQ29yZS9sb2FkZXIvY2FjaGUvQ2FjaGVkSW1h Z2UuaAkod29ya2luZyBjb3B5KQpAQCAtNDcsNyArNDcsNyBAQCBwdWJsaWM6CiAgICAgSW1hZ2Uq IGltYWdlKCkgY29uc3Q7IC8vIFJldHVybnMgdGhlIG51bGxJbWFnZSgpIGlmIHRoZSBpbWFnZSBp cyBub3QgYXZhaWxhYmxlIHlldC4KICAgICBib29sIGhhc0ltYWdlKCkgY29uc3QgeyByZXR1cm4g bV9pbWFnZS5nZXQoKTsgfQogCi0gICAgSW1hZ2UqIGJyb2tlbkltYWdlKGZsb2F0IGRldmljZVNj YWxlRmFjdG9yKSBjb25zdDsKKyAgICBzdGQ6OnBhaXI8SW1hZ2UqLCBmbG9hdD4gYnJva2VuSW1h Z2UoZmxvYXQgZGV2aWNlU2NhbGVGYWN0b3IpIGNvbnN0OyAvLyBSZXR1cm5zIGFuIGltYWdlIGFu ZCB0aGUgaW1hZ2UncyByZXNvbHV0aW9uIHNjYWxlIGZhY3Rvci4KICAgICBib29sIHdpbGxQYWlu dEJyb2tlbkltYWdlKCkgY29uc3Q7IAogCiAgICAgYm9vbCBjYW5SZW5kZXIoZmxvYXQgbXVsdGlw bGllcikgY29uc3QgeyByZXR1cm4gIWVycm9yT2NjdXJyZWQoKSAmJiAhaW1hZ2VTaXplKG11bHRp cGxpZXIpLmlzRW1wdHkoKTsgfQpJbmRleDogU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRl ckltYWdlLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVy SW1hZ2UuY3BwCShyZXZpc2lvbiA5NTA1MykKKysrIFNvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9S ZW5kZXJJbWFnZS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTg3LDE2ICs4NywxMCBAQCBJbnRTaXpl IFJlbmRlckltYWdlOjppbWFnZVNpemVGb3JFcnJvcihDCiAgICAgSW50U2l6ZSBpbWFnZVNpemU7 CiAgICAgaWYgKG5ld0ltYWdlLT53aWxsUGFpbnRCcm9rZW5JbWFnZSgpKSB7CiAgICAgICAgIGZs b2F0IGRldmljZVNjYWxlRmFjdG9yID0gUGFnZTo6ZGV2aWNlU2NhbGVGYWN0b3IoZnJhbWUoKSk7 Ci0gICAgICAgIGltYWdlU2l6ZSA9IG5ld0ltYWdlLT5icm9rZW5JbWFnZShkZXZpY2VTY2FsZUZh Y3RvciktPnNpemUoKTsKLSAgICAgICAgaWYgKGRldmljZVNjYWxlRmFjdG9yID49IDIpIHsKLSAg ICAgICAgICAgIC8vIEl0IGlzIGltcG9ydGFudCB0byBzY2FsZSBieSAwLjUgaW5zdGVhZCBvZiB0 aGUgZGV2aWNlU2NhbGVGYWN0b3IgYmVjYXVzZSB0aGUKLSAgICAgICAgICAgIC8vIGhpZ2ggcmVz b2x1dGlvbiBicm9rZW4gaW1hZ2UgYXJ0d29yayBpcyBhY3R1YWxseSBhIDJ4IGltYWdlLiBXZSBz aG91bGQgCi0gICAgICAgICAgICAvLyBjb25zaWRlciBhZGRpbmcgZnVuY3Rpb25hbGl0eSB0byBJ bWFnZSB0byBhc2sgYWJvdXQgdGhlIGltYWdlJ3MgcmVzb2x1dGlvbiwKLSAgICAgICAgICAgIC8v IGFuZCB0aGVuIHdlIGNvdWxkIHNjYWxlIGJ5IDEgLyByZXNvbHV0aW9uLiBUaGlzIGlzIGEgc29s dXRpb24gdGhhdCB3b3VsZAotICAgICAgICAgICAgLy8gc2NhbGUgYmV0dGVyIHNpbmNlIHRoaXMg aGFyZGNvZGVkIG51bWJlciB3aWxsIGhhdmUgdG8gY2hhbmdlIGlmIHdlIGV2ZXIgZ2V0Ci0gICAg ICAgICAgICAvLyBhcnR3b3JrIGF0IG90aGVyLCBoaWdoZXIgcmVzb2x1dGlvbnMuIAotICAgICAg ICAgICAgaW1hZ2VTaXplLnNjYWxlKDAuNWYpOwotICAgICAgICB9CisgICAgICAgIHBhaXI8SW1h Z2UqLCBmbG9hdD4gYnJva2VuSW1hZ2VBbmRJbWFnZVNjYWxlRmFjdG9yID0gbmV3SW1hZ2UtPmJy b2tlbkltYWdlKGRldmljZVNjYWxlRmFjdG9yKTsKKyAgICAgICAgaW1hZ2VTaXplID0gYnJva2Vu SW1hZ2VBbmRJbWFnZVNjYWxlRmFjdG9yLmZpcnN0LT5zaXplKCk7CisgICAgICAgIGlmIChkZXZp Y2VTY2FsZUZhY3RvciA+PSAyKQorICAgICAgICAgICAgaW1hZ2VTaXplLnNjYWxlKDEgLyBicm9r ZW5JbWFnZUFuZEltYWdlU2NhbGVGYWN0b3Iuc2Vjb25kKTsKICAgICB9IGVsc2UKICAgICAgICAg aW1hZ2VTaXplID0gbmV3SW1hZ2UtPmltYWdlKCktPnNpemUoKTsKIApAQCAtMjg2LDEwICsyODAs MTEgQEAgdm9pZCBSZW5kZXJJbWFnZTo6cGFpbnRSZXBsYWNlZChQYWludEluZgogICAgICAgICAg ICAgaWYgKG1faW1hZ2VSZXNvdXJjZS0+ZXJyb3JPY2N1cnJlZCgpICYmICFpbWFnZS0+aXNOdWxs KCkgJiYgdXNhYmxlV2lkdGggPj0gaW1hZ2UtPndpZHRoKCkgJiYgdXNhYmxlSGVpZ2h0ID49IGlt YWdlLT5oZWlnaHQoKSkgewogICAgICAgICAgICAgICAgIGZsb2F0IGRldmljZVNjYWxlRmFjdG9y ID0gUGFnZTo6ZGV2aWNlU2NhbGVGYWN0b3IoZnJhbWUoKSk7CiAgICAgICAgICAgICAgICAgLy8g Q2FsbCBicm9rZW5JbWFnZSgpIGV4cGxpY2l0bHkgdG8gZW5zdXJlIHdlIGdldCB0aGUgYnJva2Vu IGltYWdlIGljb24gYXQgdGhlIGFwcHJvcHJpYXRlIHJlc29sdXRpb24uCi0gICAgICAgICAgICAg ICAgaW1hZ2UgPSBtX2ltYWdlUmVzb3VyY2UtPmNhY2hlZEltYWdlKCktPmJyb2tlbkltYWdlKGRl dmljZVNjYWxlRmFjdG9yKTsKKyAgICAgICAgICAgICAgICBwYWlyPEltYWdlKiwgZmxvYXQ+IGJy b2tlbkltYWdlQW5kSW1hZ2VTY2FsZUZhY3RvciA9IG1faW1hZ2VSZXNvdXJjZS0+Y2FjaGVkSW1h Z2UoKS0+YnJva2VuSW1hZ2UoZGV2aWNlU2NhbGVGYWN0b3IpOworICAgICAgICAgICAgICAgIGlt YWdlID0gYnJva2VuSW1hZ2VBbmRJbWFnZVNjYWxlRmFjdG9yLmZpcnN0OwogICAgICAgICAgICAg ICAgIEludFNpemUgaW1hZ2VTaXplID0gaW1hZ2UtPnNpemUoKTsKICAgICAgICAgICAgICAgICBp ZiAoZGV2aWNlU2NhbGVGYWN0b3IgPj0gMikKLSAgICAgICAgICAgICAgICAgICAgaW1hZ2VTaXpl LnNjYWxlKDAuNWYpOworICAgICAgICAgICAgICAgICAgICBpbWFnZVNpemUuc2NhbGUoMSAvIGJy b2tlbkltYWdlQW5kSW1hZ2VTY2FsZUZhY3Rvci5zZWNvbmQpOwogICAgICAgICAgICAgICAgIC8v IENlbnRlciB0aGUgZXJyb3IgaW1hZ2UsIGFjY291bnRpbmcgZm9yIGJvcmRlciBhbmQgcGFkZGlu Zy4KICAgICAgICAgICAgICAgICBMYXlvdXRVbml0IGNlbnRlclggPSAodXNhYmxlV2lkdGggLSBp bWFnZVNpemUud2lkdGgoKSkgLyAyOwogICAgICAgICAgICAgICAgIGlmIChjZW50ZXJYIDwgMCkK