[webkit-reviews] review granted: [Bug 71368] Switch SVGImage cache to store ImageBuffers instead of whole SVGImages, including a DOM/Render tree : [Attachment 113785] Patch v2

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 71368: Switch SVGImage cache to store ImageBuffers instead of whole
SVGImages, including a DOM/Render tree
https://bugs.webkit.org/show_bug.cgi?id=71368

Attachment 113785: Patch v2
https://bugs.webkit.org/attachment.cgi?id=113785&action=review

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


r=me as this seems to be an improvement. 

I like introduction of the bitmap cache for svg images though I'm rather
unhappy that this is done as regression fix instead of as no-behavior-changes
performace patch.

Please think carefully if this is really the architecture you need. The FIXMEs
here indicate that this still leaves major issues unsolved.

> Source/WebCore/ChangeLog:11
> +	   Fix regressions/races introduced by r98852. SVGImage repainting
didn't work under certain circumstences.
> +	   Avoid creating multiple SVGImages and caching them for different
sizes/zoom levels. Introduce SVGImageCache
> +	   which holds rendered versions of the SVGImage at certain sizes/zoom
levels. It holds (ImageBuffer, Image) pairs
> +	   for each renderer, associated with a size and zoom level.

You should explain why and when the repainting didn't work and how this patch
fixes the problem.

This patch introduces bitmap caching for SVG images, a major change. You should
talk more about performance and memory implications.

> Source/WebCore/page/DragController.cpp:659
> +    // Don't use cachedImage->imageForRenderer() here as that may return
BitmapImages for cached SVG Images.
> +    // Users of getImage() want access to the SVGImage, in order to figure
out the filename extensions,
> +    // which would be empty when asking the cached BitmapImages.
>      return (cachedImage && !cachedImage->errorOccurred()) ?

It sounds like this code should be refactored or at least renamed
(getImageForFileNameExtension()?) as it does surpising things. 

Does some test cover this?

> Source/WebCore/rendering/ImageBySizeCache.cpp:68
> -Image* ImageBySizeCache::getImage(const RenderObject* renderer, const
IntSize& size, float zoom)
> +Image* ImageBySizeCache::getImage(const RenderObject* renderer, const
IntSize& size)

Since ImageBySizeCache now has only one client, I think it should be merged
back to CSSImageGeneratorValue. It is bit too confusing as a standalone class
without clear code sharing purpose.

> Source/WebCore/svg/graphics/SVGImage.cpp:157
> +    // FIXME: This doesn't work correctly with animations. If an image
contains animations, that say run for 2 seconds,
> +    // and we currently have one <img> that displays us. If we open another
document referencing the same SVGImage it
> +    // will display the document at a time where animations already ran -
even though it has its own ImageBuffer.
> +    // We currently don't implement SVGSVGElement::setCurrentTime, and can
NOT go back in time, once animations started.
> +    // There's no way to fix this besides avoiding style/attribute mutations
from SVGAnimationElement.

How do you know that fixing this rather major issue won't require
rearchitecting everything yet again?

> Source/WebCore/svg/graphics/SVGImageCache.cpp:95
> +void SVGImageCache::redrawTimerFired(Timer<SVGImageCache>*)
> +{

I find it strange that a cache class has a redraw timer. Seems like someone
else should take care of making the calls async.

> Source/WebCore/svg/graphics/SVGImageCache.h:50
> +    void setRequestedSizeAndZoom(const RenderObject*, const IntSize&, float
zoom);
> +    void getRequestedSizeAndZoom(const RenderObject*, IntSize*, float*
zoom);

Since you define a type of size/zoom pair, you should use it everywhere.

> Source/WebCore/svg/graphics/SVGImageCache.h:59
> +    struct CachedSizeAndZoom {

Word "Cached" adds no information as this type is already scoped to
"SVGImageCache"

> Source/WebCore/svg/graphics/SVGImageCache.h:75
> +    struct CachedImageData {

Same here.

> Source/WebCore/svg/graphics/SVGImageCache.h:83
> +	   CachedImageData(ImageBuffer* newBuffer, PassRefPtr<Image> newImage,
const IntSize& newSize, float newZoom)

Could use SizeAndZoom type.

> Source/WebCore/svg/graphics/SVGImageCache.h:94
> +	   float zoom;
> +	   IntSize size;

again.

> Source/WebCore/svg/graphics/SVGImageCache.h:105
> +    typedef HashMap<const RenderObject*, CachedSizeAndZoom>
CachedSizeAndZoomMap;
> +    typedef HashMap<const RenderObject*, CachedImageData>
CachedImageDataMap;
> +
> +    SVGImage* m_svgImage;
> +    CachedSizeAndZoomMap m_cachedSizeAndZoomMap;
> +    CachedImageDataMap m_cachedImageDataMap;

Word "Cached" adds no information as this is already scoped to "SVGImageCache"