[webkit-reviews] review granted: [Bug 67819] Use high resolution platform images when the deviceScaleFactor > 1 : [Attachment 106942] Another new patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Sep 9 17:40:37 PDT 2011


Darin Adler <darin at apple.com> has granted Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 67819: Use high resolution platform images when the deviceScaleFactor > 1
https://bugs.webkit.org/show_bug.cgi?id=67819

Attachment 106942: Another new patch
https://bugs.webkit.org/attachment.cgi?id=106942&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=106942&action=review


>>> Source/WebCore/loader/cache/CachedImage.cpp:124
>>> +	 return brokenImageLoRes;
>> 
>> I think we need a comment here explaining that these two images need to be
the same size, because the 1x image will be used for layout even if the 2x
image is the one actually drawn.
> 
> I'm not sure I agree that is necessary here. There is no similar comment in
the other places where we load a 2X image.

As I understand it, in the other places we just use the 2x image. But in this
case, the layout code uses the 1x image and then drawing code substitutes the
2x image. That's a bit different. But maybe a comment will just be confusing.

>>> Source/WebCore/rendering/RenderImage.cpp:82
>>> +static float deviceScaleFactor(Frame* frame)
>> 
>> Too bad we can’t share this with RenderLayer::drawPlatformResizerImage.
Maybe we can find a place to put this where it can be shared.
> 
> I agree that would be nice.

I’m thinking that Page.h would be a reasonable place for the helper function.

>>> Source/WebCore/rendering/RenderImage.cpp:101
>>> +	 return IntSize(paddingWidth +
newImage->brokenImage(scaleFactor)->width() * style()->effectiveZoom(),
paddingHeight + newImage->brokenImage(scaleFactor)->height() *
style()->effectiveZoom());
>> 
>> Seems unfortunate to fetch the image twice. A local variable would be
better. The old code was different because image() was just an inlined getter
function.
> 
> The old image function was not an inlined getter. Please see
CachedImage::image().

OK, then even the old code should have used a local variable!

>>> Source/WebCore/rendering/RenderLayer.cpp:2445
>>> +	 cornerResizerSize.scale(1 / deviceScaleFactor);
>> 
>> This seems too generally written. The 2X image’s size should be divided by
2, and the 1-pixel image not divided at all. There’s no good reason to do the
math with the actual scale factor. Also, rounding to an integer would be a
problem in the general case.
> 
> Adam requested using the actual scale factor instead of 2, which I did in my
original patch.
> 
> As I expressed earlier, this patch is feeling very over-reviewed to me at
this point.

I missed your over-reviewed comment; I’m not sure I agree.

If Adam suggested that, then I believe he was wrong. If the scale factor was 4
we would load the 2x image and then scale its size down by 4, so so display a
half-sized resized. Not good! It would just get worse at higher resolutions.


More information about the webkit-reviews mailing list