Bug 68054 - Return an image scale factor as well as an Image* from CachedImage::brokenImage()
Summary: Return an image scale factor as well as an Image* from CachedImage::brokenIma...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-13 21:08 PDT by Beth Dakin
Modified: 2011-09-14 11:35 PDT (History)
1 user (show)

See Also:


Attachments
Patch (5.96 KB, patch)
2011-09-13 21:20 PDT, Beth Dakin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2011-09-13 21:08:20 PDT
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.
Comment 1 Beth Dakin 2011-09-13 21:20:44 PDT
Created attachment 107281 [details]
Patch
Comment 2 Darin Adler 2011-09-13 22:13:45 PDT
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.
Comment 3 Beth Dakin 2011-09-14 11:32:26 PDT
(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!
Comment 4 Beth Dakin 2011-09-14 11:35:10 PDT
Committed change with revision 95103.