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.
Created attachment 107281 [details] Patch
Comment on attachment 107281 [details] 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.
(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!
Committed change with revision 95103.