Bug 47156

Probably we don't handle images that just have an intrinsic ratio.

The problem here is that SVGImage fails to report the correct size() for SVG with a viewBox. If both height and width of the SVG are relative, the viewBox should be used to compute the correct size for a given containerSize, but I can't find the right incantation of viewBox/preserveAspectRatio etc to do this.

(In reply to comment #2) > The problem here is that SVGImage fails to report the correct size() for SVG with a viewBox. If both height and width of the SVG are relative, the viewBox should be used to compute the correct size for a given containerSize, but I can't find the right incantation of viewBox/preserveAspectRatio etc to do this. SVGFitToViewBox contains a helper metho for that task: static AffineTransform viewBoxToViewTransform(const FloatRect& viewBoxRect, const SVGPreserveAspectRatio&, float viewWidth, float viewHeight); For a given viewBox rect and a SVGPreseveAspectRatio, as well as an available width/height it gives you the AffineTransform that you need to concat to the current graphics context CTM, to render the SVG properly according to the viewBox/preserveAspectRatio settings. Does that help? (Note, I only had a quick glance at what you wrote, and didn't check the actual testcase, i'm in hurry atm...)

I have a fix for this! I got all the CSS 2.1 background-intrinsic-* tests working, without breaking any of the CSS3 background-size tests (contain/cover still work fine). I'm down to zero regressions and will prepare a fix for this soon.

Created attachment 101807 [details] Patch Uploading a first version of the patch w/o a proper ChangeLog in WebCore. Want to get some early results - especially the cr-linux-ews pixel test results! This took more than 2 weeks, was much harder than anticipated. It fixes all CSS 2.1 background-intrinsic-* sizing tests, as well allows tiled SVG background images (though zooming is broken in that case, just like for non-SVG background images - I'll fix that in a follow-up patch).

Attachment 101807 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css2..." exit_code: 1 Source/WebCore/svg/graphics/SVGImage.cpp:148: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/svg/graphics/SVGImage.cpp:170: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/rendering/RenderImage.cpp:181: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/rendering/RenderImage.cpp:182: Missing space before { [whitespace/braces] [5] Total errors found: 4 in 99 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 101807 [details] Patch Attachment 101807 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9231378

Created attachment 101812 [details] Patch v2 Trying to get cr-linux-ews to build to receive pixel test results. Not sure why win won't apply the patch.

Attachment 101812 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css2..." exit_code: 1 Source/WebCore/ChangeLog:82: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 100 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 101812 [details] Patch v2 Attachment 101812 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9231414

Created attachment 101813 [details] Patch v3 I'll write a ChangeLog during the next hours, this should be enough now to get cr-lin-ews results, hopefully...

Comment on attachment 101813 [details] Patch v3 Attachment 101813 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9230458

Created attachment 101815 [details] Patch v4 Chromium WebCore builds now, attempt to fix WebKit chromium (DragImageTest).

CCing Adam Roben, who might know whats going on with the win slave, why it doesn't apply the patch.

Comment on attachment 101815 [details] Patch v4 Attachment 101815 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9233476 New failing tests: http/tests/security/webgl-remote-read-remote-image-blocked-no-crossorigin.html css2.1/20110323/background-intrinsic-007.htm fast/backgrounds/size/contain-and-cover.html css2.1/20110323/background-intrinsic-006.htm http/tests/inspector/network/network-cachedresources-with-same-urls.html css2.1/20110323/background-intrinsic-008.htm css2.1/20110323/background-intrinsic-005.htm fast/forms/input-spinbutton-capturing.html fast/block/float/015.html css2.1/20110323/background-intrinsic-009.htm http/tests/security/canvas-remote-read-remote-image-blocked-no-crossorigin.html css2.1/20110323/background-intrinsic-002.htm css2.1/20110323/background-intrinsic-001.htm fast/canvas/webgl/css-webkit-canvas-repaint.html fast/canvas/webgl/get-active-test.html fast/forms/input-number-large-padding.html css2.1/20110323/background-intrinsic-004.htm fast/forms/input-number-events.html http/tests/security/canvas-remote-read-remote-image-blocked-then-allowed.html css2.1/20110323/background-intrinsic-003.htm

Comment on attachment 101815 [details] Patch v4 Attachment 101815 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9231867

(In reply to comment #14) > CCing Adam Roben, who might know whats going on with the win slave, why it doesn't apply the patch. I don't know why. It's hard to tell with so little output from the bot. Maybe Eric or Adam can pull more log info for us?

> I don't know why. It's hard to tell with so little output from the bot. Maybe Eric or Adam can pull more log info for us? Not sure. Maybe its an svn/git issue? I think the win bot uses svn.

Created attachment 102011 [details] Patch v5 Working on fixing regressions... Leaving out r? flag, until these are resolved.

Attachment 102011 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css2..." exit_code: 1 Source/WebCore/loader/cache/CachedImage.cpp:207: Tab found; better to use spaces [whitespace/tab] [1] Total errors found: 1 in 106 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 102011 [details] Patch v5 Attachment 102011 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9251365

Comment on attachment 102011 [details] Patch v5 Attachment 102011 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9251433 New failing tests: http/tests/security/webgl-remote-read-remote-image-blocked-no-crossorigin.html css2.1/20110323/background-intrinsic-007.htm css1/text_properties/vertical_align.html css2.1/20110323/background-intrinsic-006.htm http/tests/inspector/network/network-cachedresources-with-same-urls.html css2.1/20110323/background-intrinsic-008.htm fast/canvas/webgl/get-active-test.html fast/block/float/015.html css2.1/20110323/background-intrinsic-009.htm fast/block/positioning/replaced-inside-fixed-top-bottom.html css2.1/20110323/background-intrinsic-002.htm css2.1/20110323/background-intrinsic-001.htm fast/canvas/webgl/css-webkit-canvas-repaint.html http/tests/security/canvas-remote-read-remote-image-blocked-no-crossorigin.html css2.1/20110323/background-intrinsic-005.htm fast/backgrounds/size/contain-and-cover.html http/tests/security/canvas-remote-read-remote-image-blocked-then-allowed.html css2.1/20110323/background-intrinsic-004.htm css2.1/20110323/background-intrinsic-003.htm

Created attachment 102107 [details] Patch v6 Finished ChangeLog, patch should be ready for review now.

Attachment 102107 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css2..." exit_code: 1 Source/WebCore/loader/cache/CachedImage.cpp:207: Tab found; better to use spaces [whitespace/tab] [1] Total errors found: 1 in 102 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 102131 [details] Patch v7 Previous patch was missing three rebaselines on mac (all progressions!). This patch passes locally and has no more pixel test regressions.

Comment on attachment 102131 [details] Patch v7 Attachment 102131 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9248773 New failing tests: http/tests/security/webgl-remote-read-remote-image-blocked-no-crossorigin.html css2.1/20110323/background-intrinsic-007.htm css1/text_properties/vertical_align.html css2.1/20110323/background-intrinsic-006.htm http/tests/inspector/network/network-cachedresources-with-same-urls.html css2.1/20110323/background-intrinsic-008.htm fast/canvas/webgl/get-active-test.html fast/block/float/015.html css2.1/20110323/background-intrinsic-009.htm fast/block/positioning/replaced-inside-fixed-top-bottom.html http/tests/security/canvas-remote-read-remote-image-blocked-then-allowed.html css2.1/20110323/background-intrinsic-002.htm css2.1/20110323/background-intrinsic-001.htm fast/canvas/webgl/css-webkit-canvas-repaint.html http/tests/security/canvas-remote-read-remote-image-blocked-no-crossorigin.html css2.1/20110323/background-intrinsic-005.htm fast/backgrounds/size/contain-and-cover.html http/tests/inspector/resource-tree/resource-tree-reload.html css2.1/20110323/background-intrinsic-004.htm css2.1/20110323/background-intrinsic-003.htm

Created attachment 102624 [details] Patch v8 Updated to ToT. Anyone wants to review?

Comment on attachment 102624 [details] Patch v8 Attachment 102624 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9284924 New failing tests: http/tests/security/webgl-remote-read-remote-image-blocked-no-crossorigin.html css2.1/20110323/background-intrinsic-007.htm css1/text_properties/vertical_align.html css2.1/20110323/background-intrinsic-006.htm http/tests/inspector/network/network-cachedresources-with-same-urls.html fast/gradients/generated-gradients.html css2.1/20110323/background-intrinsic-005.htm fast/block/float/015.html css2.1/20110323/background-intrinsic-009.htm fast/block/positioning/replaced-inside-fixed-top-bottom.html css2.1/20110323/background-intrinsic-008.htm css2.1/20110323/background-intrinsic-002.htm css2.1/20110323/background-intrinsic-001.htm fast/canvas/webgl/css-webkit-canvas-repaint.html http/tests/security/canvas-remote-read-remote-image-blocked-no-crossorigin.html fast/backgrounds/size/contain-and-cover.html http/tests/inspector/resource-tree/resource-tree-reload.html http/tests/security/canvas-remote-read-remote-image-blocked-then-allowed.html css2.1/20110323/background-intrinsic-004.htm css2.1/20110323/background-intrinsic-003.htm

Marking this bug as depending on 64974. I want to resolve all issues related to the size negotiation with object/embed/iframe/... first before landing the final part for CSS image values & html:img/svg:image support. Still the patch can be reviewed as-is :-)

Quick update: In bug 67087, a first split-up from this patch was supposed to land, though that piece of code is flawed and needs to be redesigned. I'm going to take care of this next week, stay tuned! This bug has top priority for me.

Comment on attachment 102624 [details] Patch v8 Clearing flags. A new patch will be uploaded once 69416 is in.

Created attachment 110122 [details] Draft patch To aid Anttis review on bug 69416, uploading an updated master patch, w/o test changes.

Attachment 110122 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/loader/cache/CachedImage.cp..." exit_code: 1 Source/WebCore/rendering/style/StyleImage.h:108: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/style/StyleImage.h:109: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 31 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 110122 [details] Draft patch Attachment 110122 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10006244 New failing tests: http/tests/security/xssAuditor/cookie-injection.html fast/selectors/unqualified-hover-strict.html css1/text_properties/vertical_align.html fast/gradients/generated-gradients.html fast/ruby/generated-before-counter-doesnt-crash.html fast/forms/input-spinbutton-capturing.html fast/block/float/015.html fast/images/percent-height-image.html fast/repaint/block-layout-inline-children-replaced.html fast/ruby/before-table-doesnt-crash.html fast/canvas/webgl/css-webkit-canvas-repaint.html fast/block/positioning/replaced-inside-fixed-top-bottom.html fast/forms/input-number-large-padding.html fast/backgrounds/size/contain-and-cover.html fast/forms/input-number-events.html fast/forms/implicit-submission.html fast/ruby/before-block-doesnt-crash.html

Created attachment 111303 [details] Updated patch Not marking for review yet, as there's still one known regression, but I want to gather some early EWS build results.

Attachment 111303 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 Updating OpenSource Current branch master is up to date. Updating chromium port dependencies using gclient... Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107. Re-trying 'depot_tools/gclient sync' No such file or directory at Tools/Scripts/update-webkit line 104. If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 111427 [details] Final patch - v1 Finally a complete patch again, now that all previously splitted-off chunks landed. Code changes are about 53k now, 15k ChangeLogs and the rest are new tests and pixel results.

Comment on attachment 111427 [details] Final patch - v1 View in context: https://bugs.webkit.org/attachment.cgi?id=111427&action=review Looks pretty good though I can't really review the substance of the size calculations. > Source/WebCore/loader/cache/CachedImage.cpp:138 > + if (!m_image || !m_image->isSVGImage()) > + return m_image.get(); It would be nicer to separate these conditions (return 0 on first) > Source/WebCore/loader/cache/CachedImage.cpp:147 > + if (!m_image || !m_image->isSVGImage()) > + return m_image.get(); It would be nicer to separate these conditions (return 0 on first) > Source/WebCore/loader/cache/CachedImage.cpp:167 > + if (!m_image || !m_image->isSVGImage()) > + return m_image; It would be nicer to separate these conditions (return 0 on first) > Source/WebCore/loader/cache/CachedImage.cpp:172 > + IntSize size; > + if (Image* result = m_svgImageCache.imageForRenderer(renderer, &size)) > + return result; > + if (size.isEmpty()) > + return m_image; Why isn't a zero sized image valid here? If it is not valid, why was it cached? Why return m_image instead? Could this ASSERT(!size.isEmpty()) instead? > Source/WebCore/rendering/RenderBoxModelObject.cpp:786 > +static inline LayoutSize calculateImageIntrinsicDimensions(const RenderBoxModelObject* box, StyleImage* image, const LayoutSize& positioningAreaSize) > +{ Hyatt should check this as well. The function is too long. It should be split into logically named subfunctions. > Source/WebCore/rendering/RenderBoxModelObject.cpp:824 > + // If the image has one of either an intrinsic width or an intrinsic height: > + // * and an intrinsic aspect ratio, then the missing dimension is calculated from the given dimension and the ratio. > + // * and no intrinsic aspect ratio, then the missing dimension is assumed to be the size of the rectangle that > + // establishes the coordinate system for the 'background-position' property. > + if ((resolvedWidth && !resolvedHeight) || (!resolvedWidth && resolvedHeight)) { > + if (intrinsicRatio.isEmpty()) { > + if (resolvedWidth) > + resolvedHeight = positioningAreaSize.height(); > + else > + resolvedWidth = positioningAreaSize.width(); > + } else { > + // FIXME: Remove unnecessary rounding when layout is off ints: webkit.org/b/63656 > + if (resolvedWidth) > + resolvedHeight = static_cast<LayoutUnit>(ceilf(resolvedWidth * intrinsicRatio.height() / intrinsicRatio.width())); > + else > + resolvedWidth = static_cast<LayoutUnit>(ceilf(resolvedHeight * intrinsicRatio.width() / intrinsicRatio.height())); > + } > + > + return LayoutSize(resolvedWidth, resolvedHeight); > + } This path should be a function. > Source/WebCore/rendering/RenderBoxModelObject.cpp:853 > + } This path should be a function.

Comment on attachment 111427 [details] Final patch - v1 Attachment 111427 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10126237 New failing tests: fast/repaint/block-layout-inline-children-replaced.html css2.1/20110323/background-intrinsic-007.htm css1/text_properties/vertical_align.html css2.1/20110323/background-intrinsic-006.htm css2.1/20110323/background-intrinsic-008.htm css2.1/20110323/background-intrinsic-005.htm fast/block/float/015.html css2.1/20110323/background-intrinsic-009.htm fast/block/positioning/replaced-inside-fixed-top-bottom.html css2.1/20110323/background-intrinsic-002.htm css2.1/20110323/background-intrinsic-001.htm fast/backgrounds/size/contain-and-cover.html fast/forms/implicit-submission.html css2.1/20110323/background-intrinsic-004.htm css2.1/20110323/background-intrinsic-003.htm

(In reply to comment #38) > > Source/WebCore/loader/cache/CachedImage.cpp:167 > > + if (!m_image || !m_image->isSVGImage()) > > + return m_image; > > It would be nicer to separate these conditions (return 0 on first) Fixed, all occurrences. > > > Source/WebCore/loader/cache/CachedImage.cpp:172 > > + IntSize size; > > + if (Image* result = m_svgImageCache.imageForRenderer(renderer, &size)) > > + return result; > > + if (size.isEmpty()) > > + return m_image; > > Why isn't a zero sized image valid here? If it is not valid, why was it cached? Why return m_image instead? Could this ASSERT(!size.isEmpty()) instead? I guess you misread the code. The m_svgImageCache.imageForRenderer() call looks up the overridden container size for the passed in renderer. If the renderer is known, size will be non-zero. That doesn't necessarily mean that a cached image is also available! imageForRenderer() will return 0, if it knows the size, but the Image of that desired size hasn't been produced yet. If the renderer is completely unknown imageForRenderer() will also return 0, but also size is empty. To rephrase: lookupOrCreateImageRenderer() will return m_image, if there is no associated container size for a given renderer. > > Source/WebCore/rendering/RenderBoxModelObject.cpp:786> > +{ > > Hyatt should check this as well. > > The function is too long. It should be split into logically named subfunctions. Okay, will do.

Created attachment 111450 [details] Final patch - v2 Fixed Anttis comments.

Created attachment 112504 [details] Final patch - v3 CSS Gradients + zooming was broken, it turns out there is no pixel test for that, so I added one. Made few adjustments and cleaned up the zooming related code to make it work for generated images, cached images and cached SVG images. SVG Images are zoomed by adjusting their page zoom factor. Cached non-SVG images are scaled up, and generated images render at any desired size. Previously zooming was only handled by adjusting the size of the root <svg> renderer, but that does not work at all with relative lengths. The new behaviors match Opera / Firefox / IE9, finally. We're still the only one to support the CSS 2.1 background-intrinsic* tests perfectly - checked using latest nightlies of these browsers, except IE.

Comment on attachment 112504 [details] Final patch - v3 View in context: https://bugs.webkit.org/attachment.cgi?id=112504&action=review r=me, looks good to me! > Source/WebCore/rendering/RenderBoxModelObject.cpp:842 > + // A generated image without a fixed size, will always return the container size as intrinsic size. It would be good to have a spec link here

(In reply to comment #43) > (From update of attachment 112504 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=112504&action=review > > r=me, looks good to me! > > > Source/WebCore/rendering/RenderBoxModelObject.cpp:842 > > + // A generated image without a fixed size, will always return the container size as intrinsic size. > > It would be good to have a spec link here Fixed. Landed in r98483. I will close all fixed bugs, if no regressions appear anywhere.

Reverted r98483 for reason: Change is causing some crashes and ASSERT. Committed r98508: <http://trac.webkit.org/changeset/98508>

Created attachment 112991 [details] Next try - v1 This obsoletes the "Final patch - v3" which was landed and reverted. It turns out there was a race condition, when more than one client accessed the resource. Solving it requires aligning CachedImages usage of ImageBySizeCache with the CSSImageGeneratorValue approach, which is the only safe one. Awaiting EWS results, to see whether flakiness is gone as well.

Attachment 112991 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css2..." exit_code: 1 Source/WebCore/rendering/ImageBySizeCache.h:61: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/svg/graphics/SVGImage.cpp:126: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 2 in 116 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 112991 [details] Next try - v1 Attachment 112991 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10238696 New failing tests: fast/repaint/block-layout-inline-children-replaced.html css2.1/20110323/background-intrinsic-007.htm css1/text_properties/vertical_align.html css2.1/20110323/background-intrinsic-006.htm css2.1/20110323/background-intrinsic-008.htm css2.1/20110323/background-intrinsic-005.htm fast/block/float/015.html css2.1/20110323/background-intrinsic-009.htm fast/block/positioning/replaced-inside-fixed-top-bottom.html css2.1/20110323/background-intrinsic-002.htm css2.1/20110323/background-intrinsic-001.htm css2.1/20110323/background-intrinsic-004.htm fast/backgrounds/size/contain-and-cover-zoomed.html css2.1/20110323/background-intrinsic-003.htm

Created attachment 112993 [details] Next try - v2 Fixed style issues, updated to ToT again. No more known regressions, running all svg/as-image, as-background-image, zoom/page tests in one WebProcess works fine, including wild zooming and reloading :-) Unfortunately I never saw the backtrace that Julien captured on my own. When the patch landed a DoS attack hosed build.webkit.org, so I couldn't receive any logs. Hopefully it's gone now - at least we're not deleting SVGImages anymore from within setContainerSizeForRenderer, which was the root of the problem according to the backtrace. Let's see...

Comment on attachment 112993 [details] Next try - v2 Attachment 112993 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10245144 New failing tests: css2.1/20110323/background-intrinsic-007.htm css2.1/20110323/background-intrinsic-004.htm css2.1/20110323/background-intrinsic-006.htm css2.1/20110323/background-intrinsic-008.htm css2.1/20110323/background-intrinsic-005.htm css2.1/20110323/background-intrinsic-009.htm fast/block/positioning/replaced-inside-fixed-top-bottom.html css2.1/20110323/background-intrinsic-002.htm css2.1/20110323/background-intrinsic-001.htm fast/backgrounds/size/contain-and-cover-zoomed.html css2.1/20110323/background-intrinsic-003.htm

Created attachment 113027 [details] Next try - v3 Stupid me, only included LayoutTests in the last diff, forgot Source/.

Comment on attachment 113027 [details] Next try - v3 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.

Comment on attachment 113027 [details] Next try - v3 Attachment 113027 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10240714 New failing tests: fast/repaint/block-layout-inline-children-replaced.html css2.1/20110323/background-intrinsic-007.htm css1/text_properties/vertical_align.html css2.1/20110323/background-intrinsic-006.htm css2.1/20110323/background-intrinsic-008.htm css2.1/20110323/background-intrinsic-005.htm fast/block/float/015.html css2.1/20110323/background-intrinsic-009.htm fast/block/positioning/replaced-inside-fixed-top-bottom.html css2.1/20110323/background-intrinsic-002.htm css2.1/20110323/background-intrinsic-001.htm css2.1/20110323/background-intrinsic-004.htm fast/backgrounds/size/contain-and-cover-zoomed.html css2.1/20110323/background-intrinsic-003.htm

(In reply to comment #52) > (From update of attachment 113027 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113027&action=review > > r=me, but some comments to ponder in the future. > 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. As discussed with Antti, this can be done in a follow-up cleanup. > > Source/WebCore/rendering/ImageBySizeCache.h:37 > > + , desiredSize(newSize) > > I think "requested" would be better than "desired" . Fixed. > 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. Will include that in a further patch.

Comment on attachment 113027 [details] Next try - v3 Landed in r98852. Clearing flags.

Closing master bug. Still waiting for all bots to complete testing this.

(In reply to comment #55) > (From update of attachment 113027 [details]) > Landed in r98852. Clearing flags. It broke Qt Linux Release minimal build: ../../../Source/WebCore/rendering/RenderImage.cpp: In member function 'virtual WebCore::RenderBox* WebCore::RenderImage::embeddedContentBox() const': ../../../Source/WebCore/rendering/RenderImage.cpp:549: error: 'SVGImage' was not declared in this scope ../../../Source/WebCore/rendering/RenderImage.cpp:549: error: no matching function for call to 'static_pointer_cast(WTF::RefPtr<WebCore::Image>&)' Could you fix minimal build please?

Mac rebaseline done in http://trac.webkit.org/changeset/98901.

(In reply to comment #57) > (In reply to comment #55) > > (From update of attachment 113027 [details] [details]) > > Landed in r98852. Clearing flags. > > It broke Qt Linux Release minimal build: > ../../../Source/WebCore/rendering/RenderImage.cpp: In member function 'virtual WebCore::RenderBox* WebCore::RenderImage::embeddedContentBox() const': > ../../../Source/WebCore/rendering/RenderImage.cpp:549: error: 'SVGImage' was not declared in this scope > ../../../Source/WebCore/rendering/RenderImage.cpp:549: error: no matching function for call to 'static_pointer_cast(WTF::RefPtr<WebCore::Image>&)' > > Could you fix minimal build please? Buildfix landed in http://trac.webkit.org/changeset/98895