[webkit-reviews] review granted: [Bug 47156] CSS 2.1 failure: background-intrinsic-* : [Attachment 113027] Next try - v3

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 47156: CSS 2.1 failure: background-intrinsic-*
https://bugs.webkit.org/show_bug.cgi?id=47156

Attachment 113027: Next try - v3
https://bugs.webkit.org/attachment.cgi?id=113027&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=113027&action=review


r=me, but some comments to ponder in the future.

> Source/WebCore/loader/cache/CachedImage.cpp:168
> +    // Request desired size/zoom for this renderer from the cache.
> +    IntSize size;
> +    float zoom = 1;
> +    m_svgImageCache.getDesiredSizeAndZoom(renderer, size, zoom);
> +    if (size.isEmpty())
> +	   return m_image.get();
> +
> +    if (Image* image = m_svgImageCache.getImage(renderer, size, zoom))
> +	   return image;
> +
> +    // Create and cache new image at desired size.
> +    RefPtr<Image> newImage = SVGImage::createWithDataAndSize(this,
m_data.get(), size, zoom);
> +    Image* newImagePtr = newImage.get();
> +    m_svgImageCache.addClient(renderer, size, zoom);
> +    m_svgImageCache.putImage(size, newImage.release());
> +    return newImagePtr;

Notice how you are pulling data our of m_svgImageCache and then using it to
call right back to the m_svgImageCache? This is a clear sign that all this code
belongs to the image cache class.

Generally the design (name included) of the ImageBySizeCache is not good as its
purpose and functionality is vague. The changes in this patch make the
situation worse.

> Source/WebCore/rendering/ImageBySizeCache.h:37
> +	   , desiredSize(newSize)

I think "requested" would be better than "desired" .

> Source/WebCore/rendering/ImageBySizeCache.h:47
> +    IntSize actualSize;
> +    IntSize desiredSize;
> +    float actualZoom;
> +    float desiredZoom;

What guarantees that the desired and actual sizes get reconciled at some point
if they differ? There is no code in ImageBySizeCache to do that or assert that.