RESOLVED FIXED 47156
CSS 2.1 failure: background-intrinsic-*
https://bugs.webkit.org/show_bug.cgi?id=47156
Summary CSS 2.1 failure: background-intrinsic-*
Simon Fraser (smfr)
Reported 2010-10-04 21:49:14 PDT
Created attachment 69751 [details] background-intrinsic-001.htm The following tests fail: html4/background-intrinsic-001 fail html4/background-intrinsic-003 fail html4/background-intrinsic-004 fail html4/background-intrinsic-005 fail html4/background-intrinsic-007 fail html4/background-intrinsic-008 fail
Attachments
background-intrinsic-001.htm (1.71 KB, text/html)
2010-10-04 21:49 PDT, Simon Fraser (smfr)
no flags
Patch (682.92 KB, patch)
2011-07-23 05:00 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Patch v2 (683.42 KB, patch)
2011-07-23 08:13 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Patch v3 (683.81 KB, patch)
2011-07-23 09:38 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Patch v4 (686.17 KB, patch)
2011-07-23 10:33 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Patch v5 (870.25 KB, patch)
2011-07-26 08:39 PDT, Nikolas Zimmermann
webkit-ews: commit-queue-
Patch v6 (869.72 KB, patch)
2011-07-27 01:56 PDT, Nikolas Zimmermann
no flags
Patch v7 (1.07 MB, patch)
2011-07-27 05:19 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Patch v8 (1.07 MB, patch)
2011-08-02 01:28 PDT, Nikolas Zimmermann
no flags
Draft patch (73.57 KB, patch)
2011-10-07 04:15 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Updated patch (1.94 MB, patch)
2011-10-17 12:38 PDT, Nikolas Zimmermann
no flags
Final patch - v1 (1.07 MB, patch)
2011-10-18 05:25 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Final patch - v2 (1.07 MB, patch)
2011-10-18 09:17 PDT, Nikolas Zimmermann
no flags
Final patch - v3 (deleted)
2011-10-26 07:33 PDT, Nikolas Zimmermann
koivisto: review+
Next try - v1 (deleted)
2011-10-30 02:56 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Next try - v2 (deleted)
2011-10-30 07:16 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Next try - v3 (deleted)
2011-10-31 02:58 PDT, Nikolas Zimmermann
no flags
Simon Fraser (smfr)
Comment 1 2011-01-08 13:50:20 PST
Probably we don't handle images that just have an intrinsic ratio.
Simon Fraser (smfr)
Comment 2 2011-01-08 17:05:52 PST
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.
Nikolas Zimmermann
Comment 3 2011-01-09 04:19:30 PST
(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...)
Nikolas Zimmermann
Comment 4 2011-07-04 08:59:25 PDT
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.
Nikolas Zimmermann
Comment 5 2011-07-23 05:00:08 PDT
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).
WebKit Review Bot
Comment 6 2011-07-23 05:04:12 PDT
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.
WebKit Review Bot
Comment 7 2011-07-23 05:15:25 PDT
Comment on attachment 101807 [details] Patch Attachment 101807 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9231378
Nikolas Zimmermann
Comment 8 2011-07-23 08:13:04 PDT
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.
WebKit Review Bot
Comment 9 2011-07-23 08:17:55 PDT
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.
WebKit Review Bot
Comment 10 2011-07-23 08:29:15 PDT
Comment on attachment 101812 [details] Patch v2 Attachment 101812 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9231414
Nikolas Zimmermann
Comment 11 2011-07-23 09:38:50 PDT
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...
WebKit Review Bot
Comment 12 2011-07-23 10:02:27 PDT
Comment on attachment 101813 [details] Patch v3 Attachment 101813 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9230458
Nikolas Zimmermann
Comment 13 2011-07-23 10:33:28 PDT
Created attachment 101815 [details] Patch v4 Chromium WebCore builds now, attempt to fix WebKit chromium (DragImageTest).
Nikolas Zimmermann
Comment 14 2011-07-23 10:34:52 PDT
CCing Adam Roben, who might know whats going on with the win slave, why it doesn't apply the patch.
WebKit Review Bot
Comment 15 2011-07-23 17:11:52 PDT
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
Early Warning System Bot
Comment 16 2011-07-24 20:58:52 PDT
Adam Roben (:aroben)
Comment 17 2011-07-25 05:24:59 PDT
(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?
Adam Barth
Comment 18 2011-07-25 11:07:09 PDT
> 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.
Nikolas Zimmermann
Comment 19 2011-07-26 08:39:36 PDT
Created attachment 102011 [details] Patch v5 Working on fixing regressions... Leaving out r? flag, until these are resolved.
WebKit Review Bot
Comment 20 2011-07-26 08:44:57 PDT
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.
Early Warning System Bot
Comment 21 2011-07-26 09:20:43 PDT
WebKit Review Bot
Comment 22 2011-07-26 13:59:43 PDT
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
Nikolas Zimmermann
Comment 23 2011-07-27 01:56:09 PDT
Created attachment 102107 [details] Patch v6 Finished ChangeLog, patch should be ready for review now.
WebKit Review Bot
Comment 24 2011-07-27 02:01:51 PDT
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.
Nikolas Zimmermann
Comment 25 2011-07-27 05:19:40 PDT
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.
WebKit Review Bot
Comment 26 2011-07-27 07:21:19 PDT
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
Nikolas Zimmermann
Comment 27 2011-08-02 01:28:14 PDT
Created attachment 102624 [details] Patch v8 Updated to ToT. Anyone wants to review?
WebKit Review Bot
Comment 28 2011-08-02 21:27:28 PDT
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
Nikolas Zimmermann
Comment 29 2011-08-04 05:30:55 PDT
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 :-)
Nikolas Zimmermann
Comment 30 2011-09-28 11:18:09 PDT
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.
Nikolas Zimmermann
Comment 31 2011-10-05 06:39:31 PDT
Comment on attachment 102624 [details] Patch v8 Clearing flags. A new patch will be uploaded once 69416 is in.
Nikolas Zimmermann
Comment 32 2011-10-07 04:15:39 PDT
Created attachment 110122 [details] Draft patch To aid Anttis review on bug 69416, uploading an updated master patch, w/o test changes.
WebKit Review Bot
Comment 33 2011-10-07 04:18:05 PDT
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.
WebKit Review Bot
Comment 34 2011-10-07 10:25:09 PDT
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
Nikolas Zimmermann
Comment 35 2011-10-17 12:38:03 PDT
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.
WebKit Review Bot
Comment 36 2011-10-17 12:41:21 PDT
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.
Nikolas Zimmermann
Comment 37 2011-10-18 05:25:13 PDT
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.
Antti Koivisto
Comment 38 2011-10-18 06:19:49 PDT
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.
WebKit Review Bot
Comment 39 2011-10-18 07:13:41 PDT
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
Nikolas Zimmermann
Comment 40 2011-10-18 07:54:30 PDT
(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.
Nikolas Zimmermann
Comment 41 2011-10-18 09:17:32 PDT
Created attachment 111450 [details] Final patch - v2 Fixed Anttis comments.
Nikolas Zimmermann
Comment 42 2011-10-26 07:33:09 PDT
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.
Antti Koivisto
Comment 43 2011-10-26 08:13:21 PDT
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
Nikolas Zimmermann
Comment 44 2011-10-26 08:31:43 PDT
(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.
Julien Chaffraix
Comment 45 2011-10-26 12:00:14 PDT
Reverted r98483 for reason: Change is causing some crashes and ASSERT. Committed r98508: <http://trac.webkit.org/changeset/98508>
Nikolas Zimmermann
Comment 46 2011-10-30 02:56:32 PDT
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.
WebKit Review Bot
Comment 47 2011-10-30 03:02:49 PDT
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.
WebKit Review Bot
Comment 48 2011-10-30 04:37:18 PDT
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
Nikolas Zimmermann
Comment 49 2011-10-30 07:16:30 PDT
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...
WebKit Review Bot
Comment 50 2011-10-30 11:56:49 PDT
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
Nikolas Zimmermann
Comment 51 2011-10-31 02:58:39 PDT
Created attachment 113027 [details] Next try - v3 Stupid me, only included LayoutTests in the last diff, forgot Source/.
Antti Koivisto
Comment 52 2011-10-31 03:41:24 PDT
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.
WebKit Review Bot
Comment 53 2011-10-31 06:50:02 PDT
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
Nikolas Zimmermann
Comment 54 2011-10-31 07:13:17 PDT
(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.
Nikolas Zimmermann
Comment 55 2011-10-31 07:13:57 PDT
Comment on attachment 113027 [details] Next try - v3 Landed in r98852. Clearing flags.
Nikolas Zimmermann
Comment 56 2011-10-31 08:34:25 PDT
Closing master bug. Still waiting for all bots to complete testing this.
Csaba Osztrogonác
Comment 57 2011-10-31 15:50:06 PDT
(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?
Ryosuke Niwa
Comment 58 2011-10-31 16:12:26 PDT
Csaba Osztrogonác
Comment 59 2011-10-31 17:03:10 PDT
(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
Note You need to log in before you can comment on or make changes to this bug.