Bug 105097 – SVG images are sized incorrectly on first load

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Philip Rogers]

On first load, SVG images that depend on the image size will be sized incorrectly. Additionally, the SVG image cache is not used much of the time because it depends on a call to CachedImage::setContainerSizeForRenderer(...) that it never receives.

Patch forthcoming!

[Philip Rogers]

Created attachment 179602 [details]
First pass - Queue container size requests while images are loading.

[Dean Jackson]

Comment on attachment 179602 [details]
First pass - Queue container size requests while images are loading.

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

> Source/WebCore/ChangeLog:33
> +        (CachedImage):

Nit: remove this line

[Eric Seidel (no email)]

Comment on attachment 179602 [details]
First pass - Queue container size requests while images are loading.

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

> Source/WebCore/loader/cache/CachedImage.cpp:335
> +        for (ContainerSizeRequests::iterator it = m_pendingContainerSizeRequests.begin(); it != m_pendingContainerSizeRequests.end(); ++it)
> +            setContainerSizeForRenderer(it->key, it->value.first, it->value.second);

We should clear pending requests after we send them.

> Source/WebCore/loader/cache/CachedImage.h:64
> -    void setContainerSizeForRenderer(const RenderObject*, const IntSize&, float);
> +    void setContainerSizeForRenderer(const CachedImageClient*, const IntSize&, float);

We should just remove "renderer" from this.  It's somewhat of a layering violation IMO for the loader to know anything about the rendering tree.  This can be in a follow-up patch if you'd rather.

[Eric Seidel (no email)]

Thanks dino. :)

[Philip Rogers]

Created attachment 179868 [details]
Patch for landing

[Philip Rogers]

Thanks for the reviews Dino and Eric!

(In reply to comment #3)
> (From update of attachment 179602 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179602&action=review
> 
> > Source/WebCore/loader/cache/CachedImage.cpp:335
> > +        for (ContainerSizeRequests::iterator it = m_pendingContainerSizeRequests.begin(); it != m_pendingContainerSizeRequests.end(); ++it)
> > +            setContainerSizeForRenderer(it->key, it->value.first, it->value.second);
> 
> We should clear pending requests after we send them.
Done.

> 
> > Source/WebCore/loader/cache/CachedImage.h:64
> > -    void setContainerSizeForRenderer(const RenderObject*, const IntSize&, float);
> > +    void setContainerSizeForRenderer(const CachedImageClient*, const IntSize&, float);
> 
> We should just remove "renderer" from this.  It's somewhat of a layering violation IMO for the loader to know anything about the rendering tree.  This can be in a follow-up patch if you'd rather.

I agree; lets do it as a followup though. I filed WK105244 to track this.

[WebKit Review Bot]

Comment on attachment 179868 [details]
Patch for landing

Clearing flags on attachment: 179868

Committed r137981: <http://trac.webkit.org/changeset/137981>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.