SVG Image page scale should not be looked up in imageForRenderer or lookupOrCreateImageForContainer

[Philip Rogers]

We currently dynamically compute the SVG image page scale before drawing. This should be refactored so it is only computed once, and when the page scale changes.

[Philip Rogers]

Created attachment 188481 [details]
Refactor SVG image cache to actually store page scale

[Dirk Schulze]

Comment on attachment 188481 [details]
Refactor SVG image cache to actually store page scale

View in context: https://bugs.webkit.org/attachment.cgi?id=188481&action=review

> Source/WebCore/ChangeLog:37
> +        * loader/cache/CachedImage.cpp:
> +        (WebCore::CachedImage::switchClientsToRevalidatedResource):
> +        (WebCore):
> +        (WebCore::CachedImage::imageForRenderer):
> +        (WebCore::CachedImage::setContainerSizeForRenderer):
> +        (WebCore::CachedImage::createImage):
> +        * loader/cache/CachedImage.h:
> +        (CachedImage):
> +        (WebCore::CachedImage::ContainerSizeRequest::ContainerSizeRequest):
> +        (ContainerSizeRequest):
> +        * svg/graphics/SVGImageCache.cpp:
> +        (WebCore::SVGImageCache::setContainerSizeForRenderer):
> +        (WebCore::SVGImageCache::imageForRenderer):
> +        * svg/graphics/SVGImageCache.h:
> +        (SVGImageCache):
> +        * svg/graphics/SVGImageForContainer.h:
> +        (WebCore::SVGImageForContainer::containerSize):
> +        (SVGImageForContainer):
> +        (WebCore::SVGImageForContainer::SVGImageForContainer):

This needs inline comments.

> Source/WebCore/loader/cache/CachedImage.cpp:-160
> -#if ENABLE(SVG)

wah, that looked indeed bad.

> Source/WebCore/loader/cache/CachedImage.h:115
> +        ContainerSizeRequest(const RenderObject* newRenderer, IntSize newSize, float newZoom)
> +            : renderer(newRenderer), size(newSize), zoom(newZoom) { };

This is not formatted probably. Can you do it in new lines please?

> Source/WebCore/loader/cache/CachedImage.h:116
> +

omit the new line here after that.

> Source/WebCore/svg/graphics/SVGImageCache.cpp:62
> +    // FIXME: Are both of these scale factors required on all platforms?
> +    float scale = page->deviceScaleFactor() * page->pageScaleFactor();

Can you figure this out first? I don't want to r+ this if we are unsure.

> Source/WebCore/svg/graphics/SVGImageCache.cpp:65
> +    sizeWithoutZoom.scale(1 / containerZoom);

containerZoom can not be 0? Can you assert this?

[Philip Rogers]

Thank you for the review Dirk.

In writing up a reply I found why Timothy added the two scale factors in the first place: it was to support dynamic changes (r120953). Unfortunately, it looks like SVG as CSS patterns has regressed completely (WK110047) so I couldn't write a test for whether we regress Timothy's dynamic usecase.

I'm marking this as depending on WK110047 for now. We may have to drop this approach in the end.

[Philip Rogers]

The patch in https://bugs.webkit.org/show_bug.cgi?id=110295 solves this issue as well.

[Philip Rogers]

(In reply to comment #4)
> The patch in https://bugs.webkit.org/show_bug.cgi?id=110295 solves this issue as well.

Houston, we've landed. Closing this bug too.