[webkit-reviews] review denied: [Bug 69416] Prepare SVGImage intrinsic size negotiation: Introduce an IntSize <-> SVGImage cache in CachedImage : [Attachment 110834] Next try - v2

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 69416: Prepare SVGImage intrinsic size negotiation: Introduce an IntSize
<-> SVGImage cache in CachedImage
https://bugs.webkit.org/show_bug.cgi?id=69416

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

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


Looks generally good, my comments are mostly about naming and style.

> Source/WebCore/bindings/objc/DOM.mm:442
> -    return cachedImage->image()->getNSImage();
> +    return cachedImage->image(renderer)->getNSImage();

I think there should be a separate call for getting renderer specific version
of the image, imageForRenderer() perhaps. CacheImage::image() (instead of
image(0) as this patch does) should still return the generic image (or assert
if that doesn't make sense).

> Source/WebCore/html/HTMLImageElement.cpp:260
> -	       return m_imageLoader.image()->imageSize(1.0f).width();
> +	       return m_imageLoader.image()->imageSize(renderer(),
1.0f).width();

Similarly imageSizeForRenderer.

> Source/WebCore/loader/cache/CachedImage.cpp:217
> -void CachedImage::setImageContainerSize(const IntSize& containerSize)
> +void CachedImage::setImageContainerSize(const RenderObject* renderer, const
IntSize& containerSize)

setContainerSizeForRenderer perhaps?

> Source/WebCore/loader/cache/CachedImage.cpp:239
> +    // FIXME: Turn this on once, webkit.org/b/47156 lands.
> +#if ENABLE(SVG) && 0
> +    // SVGImages are independently cached per size in the ImageBySizeCache.
> +    RenderObjectSizeCountMap::const_iterator it =
m_svgImageCache.clients().find(renderer);
> +    if (it != m_svgImageCache.clients().end()) {
> +	   IntSize currentSize = it->second.first;
> +	   ASSERT(!currentSize.isEmpty());
> +	   if (currentSize == containerSize)
> +	       return;
> +	   m_svgImageCache.removeClient(renderer);
> +	   if (!containerSize.isEmpty())
> +	       m_svgImageCache.addClient(renderer, containerSize);
> +	   return;
> +    }

There is quite of bit of caching logic added to CachedImage here and elsewhere.
I wonder if more of it could live in the cache class.

> Source/WebCore/loader/cache/CachedImage.cpp:297
> -IntRect CachedImage::imageRect(float multiplier) const
> +IntRect CachedImage::imageRect(const RenderObject* renderer, float
multiplier)

I can't find any clients for this call in this patch. Can the method be
removed?

> Source/WebCore/loader/cache/ImageBySizeCache.cpp:22
> -#include "CSSImageGeneratorValue.h"
> +#include "ImageBySizeCache.h"

This shouldn't be under loader/cache, that is really for classes closer to
networking. rendering/ would probably be appropriate as this deals with
mappings from RenderObjects.

> Source/WebCore/loader/cache/ImageBySizeCache.cpp:66
> -Image* CSSImageGeneratorValue::getImage(RenderObject* renderer, const
IntSize& size)
> +Image* ImageBySizeCache::getImage(const RenderObject* renderer, const
IntSize& size)

Not a fan of the name "ImageBySizeCache". It doesn't really capture what this
class does. From client perspective it really maps from RenderObject to Image.
I couldn't came up with any great names though.

> Source/WebCore/loader/cache/ImageBySizeCache.h:35
> +typedef pair<IntSize, int> SizeCountPair;
> +typedef HashMap<const RenderObject*, SizeCountPair>
RenderObjectSizeCountMap;

Copy code I know, but I would rather use a struct instead of pair<IntSize,
int>. The resulting code reads much better.

> Source/WebCore/platform/win/ClipboardWin.cpp:242
> -    ASSERT(image->image()->data());
> +    ASSERT(image->image(0)->data());

These all go away if you just keep image() for non-renderer specific case.