[webkit-reviews] review granted: [Bug 68054] Return an image scale factor as well as an Image* from CachedImage::brokenImage() : [Attachment 107281] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Sep 13 22:13:44 PDT 2011


Darin Adler <darin at apple.com> has granted Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 68054: Return an image scale factor as well as an Image* from
CachedImage::brokenImage()
https://bugs.webkit.org/show_bug.cgi?id=68054

Attachment 107281: Patch
https://bugs.webkit.org/attachment.cgi?id=107281&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.


More information about the webkit-reviews mailing list