Bug 68054 – Return an image scale factor as well as an Image* from CachedImage::brokenImage()

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Beth Dakin]

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.

[Beth Dakin]

Created attachment 107281 [details]
Patch

[Darin Adler]

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.

[Beth Dakin]

(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!

[Beth Dakin]

Committed change with revision 95103.